-
Notifications
You must be signed in to change notification settings - Fork 239
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
[main] Adding round robin retry for snippet script src loading #2131
Conversation
AISKU/src/Snippet.ts
Outdated
@@ -15,4 +15,5 @@ export interface Snippet { | |||
queue?: Array<() => void>; | |||
sv?: string; | |||
version?: number; | |||
cr?: boolean; // cdn retry would be proceed if ture |
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.
Once we have tested this, we will (I think) want this to be the default, so then if someone doesn't want to retry they would be required to set this to false
and then we just change if (appInsights.cr){
to if (appInsights.cr !== false){
so that anything other then false
would cause the retry to occur.
@@ -194,12 +206,33 @@ declare var cfg:ISnippetConfig; | |||
}); | |||
// let message = "Load Version 2 SDK instead to support IE"; // where to report this error? | |||
} | |||
|
|||
if (cfg.cr){ |
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.
using appinsights.cr here does not work, it does not read from the
src: "https://js3.monitor.azure.com/scripts/b/ai.2.min.js", crossOrigin: "anonymous", cfg: { // Application Insights Configuration connectionString: "YOUR_CONNECTION_STRING" }, cr:true });
@@ -8,6 +8,7 @@ export interface ISnippetConfig { | |||
crossOrigin?: string; | |||
onInit?: any; | |||
cfg: IConfiguration; | |||
cr?: boolean; |
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.
add cr here could enable getting cfg.cr in snippet.ts
@@ -98,7 +98,7 @@ declare var cfg:ISnippetConfig; | |||
let ingest = conString[strIngestionendpoint]; | |||
let endpointUrl = ingest ? ingest + "/v2/track" : aiConfig.endpointUrl; // only add /v2/track when from connectionstring | |||
|
|||
let message = "SDK LOAD Failure: Failed to load Application Insights SDK script (See stack for details)"; | |||
let message = "SDK LOAD Failure: Failed to load Application Insights SDK script via " + targetSrc + " (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 don't think we need to add the targetSrc here as it gets included in the message details via the _createException
and _createInternal
, so this becomes a duplication
@@ -194,12 +207,29 @@ declare var cfg:ISnippetConfig; | |||
}); | |||
// let message = "Load Version 2 SDK instead to support IE"; // where to report this error? | |||
} | |||
|
|||
if (cfg.cr){ |
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 think we want this to be if (cfg.cr !== false) {
so that when anything else (undefined, true, etc) it will perform the retry
handled = true; | ||
_reportFailure(targetSrc); | ||
// start retry | ||
if (domainRetryIndex >= 0 && domainRetryCount < domains.length){ |
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.
Nice handling of when they don't use one of our supported domains (I almost missed that), maybe add a comment here so that someone in the future doesn't wonder (too much) why we are checking whether domainRetryIndex
< 0.
// start retry | ||
if (domainRetryIndex >= 0 && domainRetryCount < domains.length){ | ||
let nextIdx = (domainRetryIndex + domainRetryCount + 1) % domains.length; | ||
_createScript("https://" + domains[nextIdx] + "/scripts/b/ai.2.min.js"); |
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.
But this bit should not always default to /scripts/b/ai.2.min.js
it should always be whatever they supplied as it could also be
- version 3
- one of the other formats (like
ai.2.gbl.min.js
)
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.
So maybe a regex like the IE fallback
_createScript(targetSrc.replace(/^(.*\/\/)([\w\.]*)(\/.*)$/, function (_all, http, domain, qs) {
return http + domains[nextIdx] + qs;
});
@@ -194,12 +207,31 @@ declare var cfg:ISnippetConfig; | |||
}); | |||
// let message = "Load Version 2 SDK instead to support IE"; // where to report this error? | |||
} | |||
|
|||
if (cfg.cr != false){ |
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 should be !==
so that it's an exact match, otherwise the value of cfg.cr
will get coerced to get compared against false
No description provided.