avoid new relic in IE10#9260
Conversation
|
@davidsbailey @wjordan As reviewers of Phil's PR, could you take a look at this. Any idea how I could test on my localhost? |
|
@Bjvanminnen My general strategy for testing prod-only newrelic changes has been to test on production-console. If you need to be logged in to test your changes, then an added step of pointing a load balancer at it may be needed. |
| result = "<!--[if !IE]><!--> " + | ||
| "#{script_tag} " + | ||
| "<script>if (navigator.userAgent.match('MSIE 10.0;')) { delete window.NREUM; }</script>" + | ||
| "<!--<![endif]-->" |
There was a problem hiding this comment.
where is window.NREUM coming from -- did you find the source code containing it somewhere, or did you have to inspect what was rendered by script_tag ?
There was a problem hiding this comment.
I inspected what was rendered. I do worry a little that this could change underneath us.
There was a problem hiding this comment.
seems reasonable. worst case, we start seeing new relic errors in IE10 again, right?
|
LGTM |
|
Semi-tested on production-console by making sure that (a) my additional code appeared (b) it didn't have syntax errors and (c) window.NREUM was still defined in Chrome. Did not test that IE10 successfully disables newrelic, because to use code-studio on production-console, I need to use a virtual host and don't know how to do that on IE10. I don't love merging this without more thorough testing, but I'm inclined to merge and keep a close eye on the next DTP. |
|
SGTM
|
|
@breville as dotd tomorrow, can you ping me when we DTP so i can validate? thanks |
|
This results in the following error on IE10: Apparently NREUM doesn't exist here (window.newrelic does). Given that I apparently don't understand this as well as I'd like, and this fix didn't work, I'm going to just revert this PR. I don't think there's any need to hot fix this revert as there's no clear user bad. |
|
changing this PR to delete window.newrelic, and then testing it on On Fri, Jul 1, 2016 at 2:11 PM, Brent Van Minnen notifications@github.com
|
|
One can apparently only hit code-studio on production-console by setting a particular header (Host: studio.code.org). I got around this on chrome by using https://chrome.google.com/webstore/detail/virtual-hosts/aiehidpclglccialeifedhajckcpedom?hl=en but don't have a way to do that on IE. |
|
perhaps you can get around this by naming the load balancer public url something like production-console-studio.code.org? AFAIK you only need the "Host" header so that cookies will be on code.org. |
|
I was using the public DNS for production-console (which I think means I'm skipping the load balancer?). When doing this, it takes me to the code.org site, unless I add the Host header (at which point I go to code-studio). |
|
correct, using the public DNS for production-console means skipping the On Tue, Jul 5, 2016 at 11:30 AM, Brent Van Minnen notifications@github.com
|
dc3b253
In the above commit, Phil made it so that the new relic script wouldn't even be included in IE <= 9 using conditional comments. Unfortunately, those don't work on IE10+. My solution here is to add some additional scripting that blows away the new relic object.
Unfortunately, I haven't been able to figure out how to test on localhost, where new relic is never included. Let me know if you have any ideas on how I might do this.