-
Notifications
You must be signed in to change notification settings - Fork 240
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
Followup for [Beta][Task]15982357: Dynamic config for Properties extension #1972 #1976
Conversation
MSNev
commented
Jan 27, 2023
- Add ability to tag Dynamic Config properties as referenced to support updating extensionConfig in-place with a new object instance
- Add IUnloadHookContainer to allow passing extension level unload hook container to sub-components
@@ -64,13 +64,12 @@ export class SessionManagerTests extends AITestClass { | |||
test: () => { | |||
|
|||
var sessionPrefix = newId(); | |||
var config = { | |||
var config = createDynamicConfig({ |
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.
As the Session Manager now supports onConfigChange
the passed in configuration needs to already be a dynamic configuration instance (which it will be already when initialized via the Core).
So for testing we need to simulate the same situation.
} as IPropertiesConfig; | ||
|
||
core.config.extensionConfig = core.config.extensionConfig? core.config.extensionConfig : {}; | ||
core.config.extensionConfig[id] = newConfig; |
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 is the "Key" change from the previous test, where this will cause the properties of newConfig
to be copied to core.config.extensionConfig[id]
rather than getting directly assigned (and overwriting) the previous value.
This occurs because the extensionConfig
and its properties are marked as referenced (using the new ref()
) as part of the dynamic configuration.
This is required because the PropertiesPlugin passes down it's configuration to its supporting components, and if this allowed the config to be "replaced" those support components would never be notified or receive the "updated/changed" configuration values (because they would be a different object instance)
@@ -962,7 +1180,6 @@ export class PropertiesTests extends AITestClass { | |||
|
|||
private getTelemetryConfig(): IPropertiesConfig { | |||
return { | |||
instrumentationKey: "", |
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 never have been on the IPropertiesConfig
as it's never used or referenced from here, it's only ever used from the root (IConfiguration
) definition -- So the removal is just removing dead code (defaults) and definitions.
let prefix = config.sdkExtension; | ||
this.sdkVersion = (prefix ? prefix + "_" : "") + "javascript:" + Version; | ||
}); | ||
|
||
unloadHookContainer && unloadHookContainer.add(unloadHook); |
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 always need to "register" (add()
) the returned value from the onConfigChange()
because it has internal side-effects that indirectly cause the callback function (the listener) to be associated with and referenced configuration item. And if the unloadHook
is never called all of these internal references would cause this class instance to never be garbage collected, which would result in a "memory leak" (because of the possibility of multiple instances getting left behind as referenced -- lots of assumptions in this statement and this edge case)
|
||
dynamicProto(_SessionManager, self, (_self) => { | ||
|
||
if (!config) { | ||
config = ({} as any); | ||
} | ||
|
||
let unloadHook = onConfigChange(config, (details) => { | ||
_sessionExpirationMs = config.sessionExpirationMs || ACQUISITION_SPAN; |
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.
Just converting these back to local variables to avoid function calls on every usage (this was what the TODO was about)
@@ -553,6 +561,196 @@ export class DynamicConfigTests extends AITestClass { | |||
} | |||
}); | |||
|
|||
this.testCase({ |
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.
Additional tests to validate the new ref()
and rdOnly()
functionality for dynamic configuration items.
target[name] = value; | ||
} | ||
|
||
// Assign the optional flags if true |
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.
By-design that once "marked" as referenced or read-only there is no going back (ie. there is no unref
or read-write
option).
* @internal | ||
* @ignore | ||
*/ | ||
interface _IWaitingHandlers<T> { |
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.
Not used anymore
proxyFunctionAs(_self, "_addUnloadCb", () => _unloadHandlerContainer, "add"); | ||
proxyFunctionAs(_self, "_addHook", () => _hookContainer, "add"); | ||
objDefine(_self, "_unloadHooks" as keyof BaseTelemetryPlugin, { g: () => _hookContainer }); |
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 objDefine
instead of just assigning for
- supporting unload / re-initializing a plugin which will change the value or
_hookContainer
without re-defining ininitialize
- Using
objDefine
means we can explicitly enforce theread-only
nature of this property so it can never be externally changed.
8602e08
to
6ac4186
Compare
…nsion #1972 - Add ability to tag Dynamic Config properties as referenced to support updating extensionConfig in-place with a new object instance - Add IUnloadHookContainer to allow passing extension level unload hook container to sub-components
6ac4186
to
e1ca5e2
Compare