Skip to content
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

[BUG] address issues #1517 add unload listener and #1524 config from snippet #1532

Merged
merged 6 commits into from
Apr 16, 2021

Conversation

xiao-lix
Copy link
Contributor

@xiao-lix xiao-lix commented Apr 14, 2021

@xiao-lix xiao-lix requested a review from MSNev April 14, 2021 20:28
@@ -76,6 +76,22 @@ export class ApplicationInsights extends BaseTelemetryPlugin implements IAppInsi
config.enableAutoRouteTracking = stringToBoolOrDefault(config.enableAutoRouteTracking);
config.namePrefix = config.namePrefix || "";

config.maxBatchSizeInBytes = config.maxBatchSizeInBytes > 0 ? config.maxBatchSizeInBytes : 102400; // 100kb
Copy link
Collaborator

@MSNev MSNev Apr 14, 2021

Choose a reason for hiding this comment

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

Why are we doing all of these?

Setting the defaults for lines 79, 80 and 82 I get as they are not provided otherwise, but the other checks to "set" the existing value strictly to a boolean or like on line 88 to the same value doesn't seem right...

It is also really bad from a minification perspective as all of the long names on the config elements can't be compressed.

Copy link
Collaborator

@MSNev MSNev Apr 14, 2021

Choose a reason for hiding this comment

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

ok, just went and looked at the bug and the implementation...
As the usage within the code iterates over the list of "keys" on the object returned by getDefaultConfig(), this sort of works -- it's just not very nice.

This is also very prone to breakage whenever we add a new config to IConfiguration -- if we don't remember about this of forget...

I think we need to rethink this pattern as the ctx.getConfig(identifier, field, value) does a couple of things that makes this a non-trival change as it looks up both the root config and the extensionConfig for the identifier...

Copy link
Collaborator

@MSNev MSNev Apr 14, 2021

Choose a reason for hiding this comment

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

Ok, so this line _self.config = ctx.getExtCfg<IConfig>(identifier) (Line 498 on my repo) deals with the "identifier" version of things as this will just return those values.... So it's just the root level ones that we are interested in that are getting missed (which is what the default stuff is effectively doing)...

I'm wondering whether we can do something really simple like... (_assignFn comes form the shims)
_self.config = __assignFn(ApplicationInsights.getDefaultConfig(), config, ctx.getExtCfg<IConfig>(identifier));

This effectively creates a new object using the combination of the 3 arguments, where the last value wins.
Internally, this is effectively the Object.assign() function, if you want to look it up.

We should still set the default values (lines 79, 80 and 82) so that they are set correctly, but the rest (I think) then won't be required.

@MSNev MSNev changed the title bug fix - add unload listener and initialize missing config items [BUG] addHousekeepingBeforeUnload should also be listening to the 'unload' event #1517 Apr 14, 2021
@MSNev MSNev changed the title [BUG] addHousekeepingBeforeUnload should also be listening to the 'unload' event #1517 [BUG] address issues #1517 add unload listener and #1524 config from snippet Apr 14, 2021
Comment on lines +79 to +81
config.enableDebug = stringToBoolOrDefault(config.enableDebug);
config.disableFlushOnBeforeUnload = stringToBoolOrDefault(config.disableFlushOnBeforeUnload);
config.disableFlushOnUnload = stringToBoolOrDefault(config.disableFlushOnUnload, config.disableFlushOnBeforeUnload);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MSNev Sorry Nev for the confusion... I see what's going on here.

For other configurations I added before, they're not analytics extension related and should not bring in here.

maxBatchSizeInBytes, maxBatchInterval, isBeaconApiDisabled, enableSessionStorageBuffer, isRetryDisabled, disableTelemetry are sender config and initialized here. And when user passes in user defined config, it receives here.

maxAjaxCallsPerView , disableCorrelationHeaders, enableCorsCorrelation, correlationHeaderExcludedDomains, disableAjaxTracking are remote dependency extension config , initialized here and received user defined here.

And disableCookiesUsage is handled by the cookie manager you created.

But for the above three configs, this is the place to initialize them. And on line 505, getDefaultConfig is missing the param config so the user defined configs dropped!

I think we need this config.[key] pattern because we don't need all keys from config, like "extensionConfig" is in config object too but we only need to initialize some of the configs... does that make sense?

@xiao-lix xiao-lix merged commit ababc00 into master Apr 16, 2021
@MSNev MSNev deleted the lxiao/listen-to-unload branch August 4, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants