Skip to content

[main] Minor fixes for handling ikey promises with dynamic changes #2500

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.

Sign up for GitHub

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

Merged
MSNev merged 1 commit into from
Apr 4, 2025
Merged

[main] Minor fixes for handling ikey promises with dynamic changes #2500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 42 additions & 40 deletions AISKU/src/AISku.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,27 @@ function _chkDiagLevel(value: number) {
return value && value > 0;
}

function _parseCs(config: IConfiguration & IConfig, configCs: string | IPromise<string>) {
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this out of the class as it's basically invariant so it doesn't need access to any of the properties in the closure.

return createSyncPromise<ConnectionString>((resolve, reject) => {
doAwaitResponse(configCs, (res) => {
let curCs = res && res.value;
let parsedCs = null;
if (!res.rejected && curCs) {
// replace cs with resolved values in case of circular promises
config.connectionString = curCs;
parsedCs = parseConnectionString(curCs);
}

// if can't resolve cs promise, null will be returned
resolve(parsedCs);
});
});
}

/**
* Application Insights API
* @group Entrypoint
* @group Classes
* @class Initialization
* @implements {IApplicationInsights}
*/
export class AppInsightsSku implements IApplicationInsights {
public snippet: Snippet;
Expand Down Expand Up @@ -200,60 +215,47 @@ export class AppInsightsSku implements IApplicationInsights {

// Will get recalled if any referenced values are changed
_addUnloadHook(onConfigChange(cfgHandler, () => {
let configCs = _config.connectionString;

function _parseCs() {
return createSyncPromise<ConnectionString>((resolve, reject) => {
doAwaitResponse(configCs, (res) => {
let curCs = res && res.value;
let parsedCs = null;
if (!res.rejected && curCs) {
// replace cs with resolved values in case of circular promises
_config.connectionString = curCs;
parsedCs = parseConnectionString(curCs);
}
// if can't resolve cs promise, null will be returned
resolve(parsedCs);
});
});

}
let configCs = _config.connectionString;

if (isPromiseLike(configCs)) {
let ikeyPromise = createSyncPromise<string>((resolve, reject) => {
_parseCs().then((cs) => {
let ikey = _config.instrumentationKey;
ikey = cs && cs.instrumentationkey || ikey;
resolve(ikey);
}).catch((e) => {
// parseCs will always resolve(unless timeout)
// return null in case any error happens
resolve(null);
doAwaitResponse(_parseCs(_config, configCs), (rsp) => {
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use doAwaitResponse as this helps with minification in several ways.

  • no then or catch names (which can't be minified)
  • Only 1 function (when using then/catch each one generates a function)

if (!rsp.rejected) {
let ikey = _config.instrumentationKey;
let cs = rsp.value;
ikey = cs && cs.instrumentationkey || ikey;
resolve(ikey);
} else {
// parseCs will always resolve(unless timeout)
// return null in case any error happens
resolve(null);
}
});

});

let url: IPromise<string> | string = _config.userOverrideEndpointUrl;
if (isNullOrUndefined(url)) {
url = createSyncPromise<string>((resolve, reject) => {
_parseCs().then((cs) => {
let url = _config.endpointUrl;
let ingest = cs && cs.ingestionendpoint;
url = ingest? ingest + DEFAULT_BREEZE_PATH : url;
resolve(url);
}).catch((e) => {
// parseCs will always resolve(unless timeout)
// return null in case any error happens
resolve(null);
doAwaitResponse(_parseCs(_config, configCs), (rsp) => {
if (!rsp.rejected) {
let url = _config.endpointUrl;
let cs = rsp.value;
let ingest = cs && cs.ingestionendpoint;
url = ingest? ingest + DEFAULT_BREEZE_PATH : url;
resolve(url);
} else {
// parseCs will always resolve(unless timeout)
// return null in case any error happens
resolve(null);
}
});

});
}

_config.instrumentationKey = ikeyPromise;
_config.endpointUrl = url;

}

if (isString(configCs) && configCs) {
// confirm if promiselike function present
// handle cs promise here
Expand Down
234 changes: 120 additions & 114 deletions shared/AppInsightsCore/src/JavaScriptSDK/AppInsightsCore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,130 +348,18 @@ export class AppInsightsCore<CfgType extends IConfiguration = IConfiguration> im
// This will be "re-run" if the referenced config properties are changed
_addUnloadHook(_configHandler.watch((details) => {
let rootCfg = details.cfg;

let isPending = _activeStatus === eActiveStatus.PENDING;

if (isPending){
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this logic around as by having this here this will cause subsequent "calls" for the onConfigChanges to not tag / reference any contained properties which would then result on the onConfigChange to never get called again (as the last run didn't reference anything)

Found this as part of adding new configuration options for the OTel beta as I was adding additional checks.

// means waiting for previous promises to be resolved, won't apply new changes
return;
}

_initInMemoMaxSize = rootCfg.initInMemoMaxSize || maxInitQueueSize;
// app Insights core only handle ikey and endpointurl, aisku will handle cs
let ikey = rootCfg.instrumentationKey;
let endpointUrl = rootCfg.endpointUrl; // do not need to validate endpoint url, if it is null, default one will be set by sender

if (isNullOrUndefined(ikey)) {
_instrumentationKey = null;
// if new ikey is null, set status to be inactive, all new events will be saved in memory or dropped
_activeStatus = ActiveStatus.INACTIVE;
let msg = "Please provide instrumentation key";

if (!_isInitialized) {
// only throw error during initialization
throwError(msg);
} else {
_throwInternal(_logger, eLoggingSeverity.CRITICAL, _eInternalMessageId.InvalidInstrumentationKey, msg);
_releaseQueues();
}
return;

}

let promises: IPromise<string>[] = [];
if (isPromiseLike(ikey)) {
promises.push(ikey);
_instrumentationKey = null; // reset current local ikey variable (otherwise it will always be the previous ikeys if timeout is called before promise cb)
} else {
// string
_instrumentationKey = ikey;
}

if (isPromiseLike(endpointUrl)) {
promises.push(endpointUrl);
_endpoint = null; // reset current local endpoint variable (otherwise it will always be the previous urls if timeout is called before promise cb)
} else {
// string or null
_endpoint = endpointUrl;
}

// at least have one promise
if (promises.length) {
// reset to false for new dynamic changes
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also broke if part into it's own function rather than inline

_isStatusSet = false;
_activeStatus = eActiveStatus.PENDING;
let initTimeout = isNotNullOrUndefined(rootCfg.initTimeOut)? rootCfg.initTimeOut : maxInitTimeout; // rootCfg.initTimeOut could be 0
let allPromises = createSyncAllSettledPromise<string>(promises);
_initTimer = scheduleTimeout(() => {
// set _isStatusSet to true
// set active status
// release queues
_initTimer = null;
if (!_isStatusSet) {
_setStatus();
}

}, initTimeout);

doAwaitResponse(allPromises, (response) => {
try {
if (_isStatusSet) {
// promises take too long to resolve, ignore them
// active status should be set by timeout already
return;
}

if (!response.rejected) {
let values = response.value;
if (values && values.length) {
// ikey
let ikeyRes = values[0];
_instrumentationKey = ikeyRes && ikeyRes.value;

// endpoint
if (values.length > 1) {
let endpointRes = values[1];
_endpoint = endpointRes && endpointRes.value;

}

}
if (_instrumentationKey) {
// if ikey is null, no need to trigger extra dynamic changes for extensions
config.instrumentationKey = _instrumentationKey; // set config.instrumentationKey for extensions to consume
config.endpointUrl = _endpoint; // set config.endpointUrl for extensions to consume
}

}

// set _isStatusSet to true
// set active status
// release queues
_setStatus();

} catch (e) {
if (!_isStatusSet){
_setStatus();
}
}

});
} else {
// means no promises
_setStatus();

}

_handleIKeyEndpointPromises(rootCfg);
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also broke these out into smaller functions to contain the logic rather than just having this inline

Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual "processing" logic should be mostly the same (I'll add a comment to the new function that triggers the next onConfigChange)


//_instrumentationKey = details.cfg.instrumentationKey;
// Mark the extensionConfig and all first level keys as referenced
// This is so that calls to getExtCfg() will always return the same object
// Even when a user may "re-assign" the plugin properties (or it's unloaded/reloaded)
let extCfg = details.ref(details.cfg, STR_EXTENSION_CONFIG);
objForEachKey(extCfg, (key) => {
details.ref(extCfg, key);
});


}));

_notificationManager = notificationManager;
Expand Down Expand Up @@ -647,6 +535,124 @@ export class AppInsightsCore<CfgType extends IConfiguration = IConfiguration> im
return _startLogPoller(true);
};

function _handleIKeyEndpointPromises(theConfig: IConfiguration) {
// app Insights core only handle ikey and endpointurl, aisku will handle cs
// But we want to reference these config values so that if any future changes are made
// this will trigger the re-run of the watch function
// and the ikey and endpointUrl will be set to the new values
let ikey = theConfig.instrumentationKey;
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines are really the key part of the referencing change, as per the comment if these direct configuration properties are not referenced as part of the subsequent call to the onConfigChange (they where originally after the "pending" check) then the dynamic configuration change usage detection doesn't identify that this function should be called (indirectly by the calling onConfigChange (watch) function) again if / when either of these configuration options change.

Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from moving these 2 referenced variables to before the pending check (and not having a isPending variable) the reset of the logic of this function "should" (please check) be the same.

Copy link
Contributor

@Karlie-777 Karlie-777 Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial design of setting ikey after pending check is to avoid the scenarios where user changes the ikey to a string value before the previous ikey promises are resolved. But this did block the following onConfigChange before all promises are resolved.

let endpointUrl = theConfig.endpointUrl; // do not need to validate endpoint url, if it is null, default one will be set by sender

// Check if we are waiting for previous promises to be resolved, won't apply new changes
if (_activeStatus !== eActiveStatus.PENDING) {
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We "probably" should really look at handling reassignment to different promises but that's a much larger (and risker) change.

if (isNullOrUndefined(ikey)) {
_instrumentationKey = null;

// if new ikey is null, set status to be inactive, all new events will be saved in memory or dropped
_activeStatus = ActiveStatus.INACTIVE;
let msg = "Please provide instrumentation key";

if (!_isInitialized) {
// only throw error during initialization
throwError(msg);
} else {
_throwInternal(_logger, eLoggingSeverity.CRITICAL, _eInternalMessageId.InvalidInstrumentationKey, msg);
_releaseQueues();
}

return;
}

let promises: IPromise<string>[] = [];
if (isPromiseLike(ikey)) {
promises.push(ikey);
_instrumentationKey = null; // reset current local ikey variable (otherwise it will always be the previous ikeys if timeout is called before promise cb)
} else {
// string
_instrumentationKey = ikey;
}

if (isPromiseLike(endpointUrl)) {
promises.push(endpointUrl);
_endpoint = null; // reset current local endpoint variable (otherwise it will always be the previous urls if timeout is called before promise cb)
} else {
// string or null
_endpoint = endpointUrl;
}

// at least have one promise
if (promises.length) {
_waitForInitPromises(theConfig, promises);
} else {
// means no promises
_setStatus();
}
}
}

function _waitForInitPromises(theConfig: IConfiguration, promises: IPromise<string>[]) {
// reset to false for new dynamic changes
_isStatusSet = false;
_activeStatus = eActiveStatus.PENDING;
let initTimeout = isNotNullOrUndefined(theConfig.initTimeOut)? theConfig.initTimeOut : maxInitTimeout; // theConfig.initTimeOut could be 0
let allPromises = createSyncAllSettledPromise<string>(promises);

if (_initTimer) {
Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is new, it cancels any previously running timer (which should never occur based on the above pending check), but just preparing and covering all bases.

Copy link
Collaborator Author

@MSNev MSNev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this function logic should be the same as previously (please check)

// Stop any previous timer
_initTimer.cancel();
}

_initTimer = scheduleTimeout(() => {
// set _isStatusSet to true
// set active status
// release queues
_initTimer = null;
if (!_isStatusSet) {
_setStatus();
}
}, initTimeout);

doAwaitResponse(allPromises, (response) => {
try {
if (_isStatusSet) {
// promises take too long to resolve, ignore them
// active status should be set by timeout already
return;
}

if (!response.rejected) {
let values = response.value;
if (values && values.length) {
// ikey
let ikeyRes = values[0];
_instrumentationKey = ikeyRes && ikeyRes.value;

// endpoint
if (values.length > 1) {
let endpointRes = values[1];
_endpoint = endpointRes && endpointRes.value;
}
}

if (_instrumentationKey) {
// if ikey is null, no need to trigger extra dynamic changes for extensions
theConfig.instrumentationKey = _instrumentationKey; // set config.instrumentationKey for extensions to consume
theConfig.endpointUrl = _endpoint; // set config.endpointUrl for extensions to consume
}
}

// set _isStatusSet to true
// set active status
// release queues
_setStatus();
} catch (e) {
if (!_isStatusSet){
_setStatus();
}
}
});
}

function _setStatus() {
_isStatusSet = true;
if (isNullOrUndefined(_instrumentationKey)) {
Expand Down
Loading