-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update snippet to support reporting script load failures #1258
Conversation
MSNev
commented
Apr 21, 2020
- New Snippet Setup pollutes the global namespace (window) New Snippet Setup pollutes the global namespace (window) #974
AISKU/snippet/snippet.js
Outdated
} | ||
(function (win, doc, snipConfig) { | ||
var locn = win.location; | ||
var helpLink = "https://aka.ms/<TBD>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be updated before I can check this in
@@ -92,6 +92,7 @@ export const _InternalMessageId = { | |||
InvalidEvent: 70, | |||
FailedMonitorAjaxSetRequestHeader: 71, | |||
SendBrowserInfoOnUserInit: 72, | |||
PluginException: 73 | |||
PluginException: 73, | |||
SnippetScriptLoadFailure: 99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not 74? 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to move it to something that was obvious and easy to recall from an ICM :-)
AISKU/src/Init.ts
Outdated
@@ -35,7 +35,7 @@ try { | |||
|
|||
// overwrite snippet with full appInsights | |||
// for 2.0 initialize only if required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also update the document here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
AISKU/snippet/snippet.js
Outdated
var appInsights = { | ||
initialize: true, // initialize sdk on download | ||
queue: [], | ||
snipVer: 3.0, // Track the actual snippet version for reporting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this snipVer need to update every time SDK updates, like all plugin versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snipVer is only for reporting the version of the snippet that people are using, so ideally yes. But it's not a critical change. I'd update it only when we either fix some critical change or add some new feature(s). So that when reported we can just say -- updaste to the latest version of the snippet if they are expecting something that is supported in a later version.
0665b69
to
c9be850
Compare
AISKU/snippet/snippet.js
Outdated
var ingest = conString[strIngestionendpoint]; | ||
var endpointUrl = ingest ? ingest + "/v2/track" : config.endpointUrl; // only add /v2/track when from connectionstring | ||
|
||
var message = "SDK LOAD Failure: Failed to load App Insights Sdk script (See stack for details)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated per Ram's email comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see App Insights. We should call this Application Insights. and capitialize Sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also add JavaScript SDK to the message, if it isn't visible anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to keep the text data to min so we can minimize on snippet bloat. Fact that we are listing .js file in the error should be good enough IMO.
AISKU/snippet/snippet.js
Outdated
typeName: "ScriptLoadFailed", | ||
message: message.replace(/\./g, "-"), // Replacing '.' characters as it causes the portal to hide the start of the message in the summary | ||
hasFullStack: false, | ||
stack: message + "\nSnippet failed to load [" + targetSrc + "] -- Telemetry is disabled\nHelp Link: " + helpLink + "\nHost: " + (locn && locn.pathname || "_unknown_"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated per Ram's email comments
AISKU/snippet/snippet.js
Outdated
var ingest = conString[strIngestionendpoint]; | ||
var endpointUrl = ingest ? ingest + "/v2/track" : config.endpointUrl; // only add /v2/track when from connectionstring | ||
|
||
var message = "SDK LOAD Failure: Failed to load App Insights Sdk script (See stack for details)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see App Insights. We should call this Application Insights. and capitialize Sdk?
} | ||
|
||
// Assigning these to local variables allows them to be minified to save space: | ||
var targetSrc = aiConfig.url || snipConfig.src; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the stuff other than what you are adding for handling snippet failures, are there any other behavior changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - Apart from "adding" the snipConfig (src, name, ld, useXhr -- see Readmd.md for documentation) which mostly are just exposing the previously internal only values -- so we just define them once
c9be850
to
6add49d
Compare
- New Snippet Setup pollutes the global namespace (window) #974
6add49d
to
4428dc0
Compare