Conversation
There was a problem hiding this comment.
Pull request overview
Implements “Phase 0” CONTEXT.md compliance updates across shared/otel-core by moving several OTel factories to explicit config objects, adding dynamic-config watching/unload hook cleanup, and expanding TypeDoc so existing implementations align with the documented validation checklist.
Changes:
- Refactors provider/factory APIs to accept explicit config objects (and exports new config interfaces from
src/index.ts). - Adds dynamic-config watching (
createDynamicConfig(...).watch(...)) +IUnloadHook.rm()cleanup paths for shutdown/disable flows. - Introduces lifecycle cleanup on resources (
IOTelResource.shutdown) and updates unit tests to construct providers with required dependencies.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/otel-core/src/otel/sdk/OTelWebSdk.ts | Passes error handlers into logger provider creation (as part of injected-deps compliance). |
| shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts | Makes logger provider config required, adds dynamic-config watch + unload cleanup, and adds TypeDoc. |
| shared/otel-core/src/otel/resource/resource.ts | Adds shutdown() to clear internal resource state and documents lifecycle behavior. |
| shared/otel-core/src/otel/api/trace/tracerProvider.ts | Renames/export refactor to createTracerProvider(config) with dynamic-config watching and shutdown cleanup. |
| shared/otel-core/src/otel/api/trace/tracer.ts | Updates tracer factory docs/signature to accept optional tracer name. |
| shared/otel-core/src/otel/api/trace/span.ts | Switches span error handling to read handlers dynamically and adds TypeDoc. |
| shared/otel-core/src/otel/api/context/contextManager.ts | Changes API to accept config object, adds dynamic-config watch, warnings, and unload cleanup on disable. |
| shared/otel-core/src/otel/api/OTelApi.ts | Updates API construction to use createTracerProvider({ host }). |
| shared/otel-core/src/interfaces/otel/trace/ITracerProviderConfig.ts | Adds new exported config interface for tracer provider creation. |
| shared/otel-core/src/interfaces/otel/resources/IOTelResource.ts | Adds optional shutdown() lifecycle hook to the resource interface. |
| shared/otel-core/src/interfaces/otel/logs/IOTelLoggerProviderConfig.ts | Makes resource and errorHandlers required; adds richer TypeDoc. |
| shared/otel-core/src/interfaces/otel/context/IContextManagerConfig.ts | Adds new exported config interface for context manager creation. |
| shared/otel-core/src/index.ts | Exports newly introduced factory + config interfaces. |
| shared/otel-core/src/ext/ValueSanitizer.ts | Import formatting update. |
| shared/otel-core/planning/IMPLEMENTATION_PLAN.md | Updates phase status markers and last-updated date. |
| shared/otel-core/Tests/Unit/src/sdk/OTelMultiLogRecordProcessor.Tests.ts | Updates tests to build logger provider with required config (resource + errorHandlers). |
| shared/otel-core/Tests/Unit/src/sdk/OTelLoggerProvider.Tests.ts | Updates tests for new required config shape and adds helper to construct valid configs. |
| shared/otel-core/Tests/Unit/src/sdk/OTelLogger.Tests.ts | Updates logger tests to construct logger provider with required config. |
| disable: () => { | ||
| activeContext = null; | ||
| enabled = false | ||
| enabled = false; | ||
|
|
||
| // Unregister config change listeners | ||
| for (let i = 0; i < _unloadHooks.length; i++) { | ||
| _unloadHooks[i].rm(); | ||
| } | ||
| _unloadHooks = []; | ||
|
|
There was a problem hiding this comment.
disable() unregisters the config watcher and clears _unloadHooks, but enable() does not re-register it. This makes enable()/disable() non-reversible with respect to config updates (after a disable/enable cycle, parentContext/errorHandlers won’t react to runtime config changes). Consider either keeping the watcher for the lifetime of the manager, or re-registering it inside enable() when needed.
|
|
||
| if (localDroppedEvents > 0) { | ||
| handleWarn(errorHandlers, "Droped " + localDroppedEvents + " events"); | ||
| handleWarn(_errorHandlers(), "Droped " + localDroppedEvents + " events"); |
There was a problem hiding this comment.
The warning message contains a typo: "Droped" should be "Dropped".
| handleWarn(_errorHandlers(), "Droped " + localDroppedEvents + " events"); | |
| handleWarn(_errorHandlers(), "Dropped " + localDroppedEvents + " events"); |
| @@ -66,7 +68,7 @@ export class OTelLoggerProviderTests extends AITestClass { | |||
| this.testCase({ | |||
| name: "LoggerProvider: constructor should use default resource when not provided", | |||
There was a problem hiding this comment.
This test name now says it "should use default resource when not provided", but the updated construction path always provides a resource via this._cfg(). Consider updating the test name (and/or scenario) so it matches what’s actually being tested after the API change.
| name: "LoggerProvider: constructor should use default resource when not provided", | |
| name: "LoggerProvider: constructor should expose configured default resource", |
| if (!_resource) { | ||
| handleError(_handlers, "Resource must be provided to LoggerProvider"); | ||
| } | ||
|
|
||
| const sharedState = createLoggerProviderSharedState( | ||
| resource, | ||
| let sharedState = createLoggerProviderSharedState( | ||
| _resource, | ||
| forceFlushTimeoutMillis, | ||
| reconfigureLimits(logRecordLimits), | ||
| config && config.processors ? config.processors : [] | ||
| config.processors || [] |
There was a problem hiding this comment.
If config.resource is missing, this only calls handleError() and then continues building the shared state with an undefined resource. That can later surface as runtime errors (e.g., log records exposing an undefined resource). Consider treating this as a hard configuration error (throw / return a noop provider) and exit early before creating sharedState.
There was a problem hiding this comment.
This looks to be a legitimate concern. Should be addressed.
| shutdown: function () { | ||
| attribContainer = null; | ||
| rawResources = null; | ||
| resolveAwaitingPromise = null; | ||
| awaitingPromise = null; | ||
| } |
There was a problem hiding this comment.
shutdown() clears awaitingPromise/resolveAwaitingPromise without resolving/rejecting any promise that may have already been returned by waitForAsyncAttributes(). Callers awaiting that promise can hang indefinitely after shutdown. Consider resolving (or rejecting) the pending promise during shutdown before nulling the references.
| let _handlers: IOTelErrorHandlers; | ||
| let parentContext: IOTelContext; | ||
|
|
||
| // Register for config changes — save the returned IUnloadHook | ||
| let _configUnload = createDynamicConfig(_cfg).watch(function () { | ||
| _handlers = _cfg.errorHandlers || {}; | ||
| parentContext = _cfg.parentContext; | ||
| }); | ||
| _unloadHooks.push(_configUnload); |
There was a problem hiding this comment.
When createContextManager() is called with no config, it still creates a dynamic config + watcher. This adds avoidable per-instance overhead, and createContextManager() appears to be used in hot paths (e.g., logger emit). Consider a fast-path for config === undefined that skips createDynamicConfig().watch() and uses default {} handlers/parentContext directly.
| let _handlers: IOTelErrorHandlers; | |
| let parentContext: IOTelContext; | |
| // Register for config changes — save the returned IUnloadHook | |
| let _configUnload = createDynamicConfig(_cfg).watch(function () { | |
| _handlers = _cfg.errorHandlers || {}; | |
| parentContext = _cfg.parentContext; | |
| }); | |
| _unloadHooks.push(_configUnload); | |
| let _handlers: IOTelErrorHandlers = {}; | |
| let parentContext: IOTelContext; | |
| // Register for config changes — save the returned IUnloadHook | |
| if (config) { | |
| let _configUnload = createDynamicConfig(_cfg).watch(function () { | |
| _handlers = _cfg.errorHandlers || {}; | |
| parentContext = _cfg.parentContext; | |
| }); | |
| _unloadHooks.push(_configUnload); | |
| } |
| // Create the logger provider using existing factory | ||
| let _loggerProvider = createLoggerProvider({ | ||
| resource: _resource, | ||
| errorHandlers: _handlers, | ||
| processors: _sdkConfig.logProcessors || [] | ||
| }); |
There was a problem hiding this comment.
createLoggerProvider() receives a new object literal with errorHandlers: _handlers. If the SDK config updates errorHandlers by replacing the object (not mutating in place), the logger provider will keep the old handlers because this config object is never updated. Consider wiring provider errorHandlers/resource to _sdkConfig (or updating this config object inside the SDK onConfigChange callback) so runtime config changes propagate consistently.
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| return sharedState.loggers.get(key)!; | ||
| return sharedState.loggers.get(key); |
There was a problem hiding this comment.
getLogger() is declared to return IOTelLogger | null, but Map.get() can return undefined. Even though the map is usually populated just above, this currently violates the function’s own contract. Consider returning sharedState.loggers.get(key) || null (or explicitly checking and returning null) to avoid leaking undefined to callers.
| return sharedState.loggers.get(key); | |
| let logger = sharedState.loggers.get(key); | |
| return logger || null; |
| getTracer(name: string, version?: string): IOTelTracer { | ||
| const tracerKey = (name|| "ai-web") + "@" + (version || "unknown"); | ||
|
|
||
| if (!tracers[tracerKey]) { | ||
| tracers[tracerKey] = _createTracer(host); | ||
| if (_isShutdown) { | ||
| handleWarn(_handlers, "A shutdown TracerProvider cannot provide a Tracer"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
getTracer() currently returns null after shutdown, but the IOTelTracerProvider.getTracer() contract returns an IOTelTracer (and the interface doc says a non-operational tracer should be returned when not operational). Consider returning a noop/non-recording tracer instead of null, or update the interface/contract consistently.
| let _unloadHooks: IUnloadHook[] = []; | ||
|
|
||
| // Register for config changes — save the returned IUnloadHook | ||
| let _configUnload = createDynamicConfig(config).watch(function () { |
There was a problem hiding this comment.
The createDynamicConfig().watch() callback updates _resource, forceFlushTimeoutMillis, logRecordLimits, and _host, but sharedState is constructed once from initial values and never mutated.
Phase 0: CONTEXT.md Compliance Fixes (Prerequisite)
Fix existing implementations to comply with CONTEXT.md before building new features.
_createTracerProvidercreateTracerProvider, add config object, add error handlers, addonConfigChangecreateLoggerProvideronConfigChangecreateContextManagercreateSpanonConfigChangeif caching config valuescreateResourceIUnloadHookmanagement with.rm()calls during shutdownDeliverable: All existing implementations pass CONTEXT.md validation checklist.