Update snippet to support reporting script load failures #1258

MSNev · 2020-04-21T20:48:34Z

New Snippet Setup pollutes the global namespace (window) New Snippet Setup pollutes the global namespace (window) #974

MSNev · 2020-04-21T22:30:09Z

AISKU/snippet/snippet.js

-    }
+(function (win, doc, snipConfig) {
+    var locn = win.location;
+    var helpLink = "https://aka.ms/<TBD>";


This still needs to be updated before I can check this in

AISKU/snippet/snippet.js

xiao-lix · 2020-04-22T05:10:39Z

shared/AppInsightsCore/src/JavaScriptSDK.Enums/LoggingEnums.ts

@@ -92,6 +92,7 @@ export const _InternalMessageId = {
    InvalidEvent: 70,
    FailedMonitorAjaxSetRequestHeader: 71,
    SendBrowserInfoOnUserInit: 72,
-    PluginException: 73
+    PluginException: 73,
+    SnippetScriptLoadFailure: 99


why is this not 74? 😬

I just wanted to move it to something that was obvious and easy to recall from an ICM :-)

xiao-lix · 2020-04-22T05:11:46Z

AISKU/src/Init.ts

@@ -35,7 +35,7 @@ try {

                // overwrite snippet with full appInsights
                // for 2.0 initialize only if required


Also update the document here?

xiao-lix · 2020-04-22T05:21:43Z

AISKU/snippet/snippet.js

+        var appInsights = {
+            initialize: true,   // initialize sdk on download
+            queue: [],
+            snipVer: 3.0,       // Track the actual snippet version for reporting.


Does this snipVer need to update every time SDK updates, like all plugin versions?

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.

MSNev · 2020-04-22T19:35:51Z

AISKU/snippet/snippet.js

+            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)";


Updated per Ram's email comments

I still see App Insights. We should call this Application Insights. and capitialize Sdk?

Would also add JavaScript SDK to the message, if it isn't visible anywhere else.

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.

MSNev · 2020-04-22T19:36:03Z

AISKU/snippet/snippet.js

+                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_"),


Updated per Ram's email comments

AISKU/snippet/snippet.js

ramthi · 2020-04-23T06:06:55Z

AISKU/snippet/snippet.js

+            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)";


I still see App Insights. We should call this Application Insights. and capitialize Sdk?

markwolff · 2020-04-23T17:36:46Z

AISKU/snippet/snippet.js

+        }
+
+        // Assigning these to local variables allows them to be minified to save space:
+        var targetSrc = aiConfig.url || snipConfig.src;


for the stuff other than what you are adding for handling snippet failures, are there any other behavior changes?

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

- New Snippet Setup pollutes the global namespace (window) #974

MSNev requested review from markwolff, ramthi, xiao-lix and hectorhdzg April 21, 2020 20:48

MSNev requested a review from a team as a code owner April 21, 2020 20:48

MSNev mentioned this pull request Apr 21, 2020

Identify whether the script is being consumed via the CDN or NPM package #1249

Merged

MSNev commented Apr 21, 2020

View reviewed changes

MSNev commented Apr 22, 2020

View reviewed changes

AISKU/snippet/snippet.js Show resolved Hide resolved

xiao-lix reviewed Apr 22, 2020

View reviewed changes

MSNev force-pushed the MSNev/UpdateSnippet branch from 0665b69 to c9be850 Compare April 22, 2020 19:33

MSNev commented Apr 22, 2020

View reviewed changes

ramthi requested changes Apr 23, 2020

View reviewed changes

markwolff reviewed Apr 23, 2020

View reviewed changes

MSNev force-pushed the MSNev/UpdateSnippet branch from c9be850 to 6add49d Compare April 23, 2020 18:54

ramthi approved these changes Apr 24, 2020

View reviewed changes

Update snippet to support reporting script load failures

4428dc0

- New Snippet Setup pollutes the global namespace (window) #974

MSNev force-pushed the MSNev/UpdateSnippet branch from 6add49d to 4428dc0 Compare April 29, 2020 19:45

MSNev merged commit 5cb9e44 into master May 1, 2020

MSNev mentioned this pull request May 1, 2020

New Snippet Setup pollutes the global namespace (window) #974

Closed

mrbullwinkle mentioned this pull request May 7, 2020

Snipped based setup discrepencies MicrosoftDocs/azure-docs#54254

Closed

MSNev added this to the 2.5.5 milestone Jun 2, 2020

MSNev linked an issue Jun 2, 2020 that may be closed by this pull request

New Snippet Setup pollutes the global namespace (window) #974

Closed

MSNev mentioned this pull request Jun 2, 2020

[Release] Increase to version 2.5.5 #1279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update snippet to support reporting script load failures #1258

Update snippet to support reporting script load failures #1258

MSNev commented Apr 21, 2020

MSNev Apr 21, 2020

xiao-lix Apr 22, 2020

MSNev Apr 22, 2020

xiao-lix Apr 22, 2020

MSNev Apr 22, 2020

xiao-lix Apr 22, 2020

MSNev Apr 22, 2020 •

edited

MSNev Apr 22, 2020

ramthi Apr 23, 2020

markwolff Apr 23, 2020

ramthi Apr 24, 2020

MSNev Apr 22, 2020

ramthi Apr 23, 2020

markwolff Apr 23, 2020

MSNev Apr 23, 2020

		@@ -35,7 +35,7 @@ try {

		// overwrite snippet with full appInsights
		// for 2.0 initialize only if required

Update snippet to support reporting script load failures #1258

Update snippet to support reporting script load failures #1258

Conversation

MSNev commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Apr 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Apr 22, 2020 •

edited