Phase 0 implementation by hectorhdzg · Pull Request #2717 · microsoft/ApplicationInsights-JS

hectorhdzg · 2026-03-20T23:53:02Z

Phase 0: CONTEXT.md Compliance Fixes (Prerequisite)

Fix existing implementations to comply with CONTEXT.md before building new features.

Component	Changes Required
`_createTracerProvider`	Rename to `createTracerProvider`, add config object, add error handlers, add `onConfigChange`
`createLoggerProvider`	Remove default config, validate required deps, get handlers from config, add `onConfigChange`
`createContextManager`	Add config object with error handlers
`createSpan`	Add `onConfigChange` if caching config values
`createResource`	Add shutdown/cleanup method
All	Add `IUnloadHook` management with `.rm()` calls during shutdown
All	Add comprehensive TypeDoc documentation

Deliverable: All existing implementations pass CONTEXT.md validation checklist.

Copilot

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.

shared/otel-core/src/otel/api/context/contextManager.ts

shared/otel-core/src/otel/api/trace/span.ts

shared/otel-core/Tests/Unit/src/sdk/OTelLoggerProvider.Tests.ts

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts

shared/otel-core/src/otel/resource/resource.ts

shared/otel-core/src/otel/api/context/contextManager.ts

shared/otel-core/src/otel/sdk/OTelWebSdk.ts

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts

shared/otel-core/src/otel/api/trace/tracerProvider.ts

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts

MSNev · 2026-03-26T23:41:31Z

shared/otel-core/src/interfaces/otel/context/IContextManagerConfig.ts

+     *
+     * @see {@link IOTelErrorHandlers}
+     */
+    errorHandlers?: IOTelErrorHandlers;


I don't think we should have this defined here, the "implementation" should inherit/use the core (Sdk/Distro) config to use the errorHandlers -- which should be provided during initialization...

Otherwise, we could end up passing the same error handlers multiple times

MSNev · 2026-03-26T23:41:49Z

shared/otel-core/src/interfaces/otel/logs/IOTelLoggerProviderConfig.ts

+     *
+     * @see {@link IOTelErrorHandlers}
+     */
+    errorHandlers: IOTelErrorHandlers;


same comment as context

MSNev · 2026-03-26T23:42:46Z

shared/otel-core/src/interfaces/otel/trace/ITracerProviderConfig.ts

+     *
+     * @see {@link IOTelErrorHandlers}
+     */
+    errorHandlers?: IOTelErrorHandlers;


same comment as context -- and this being defined for context, logs and trace reinforces my original comment

MSNev · 2026-03-26T23:43:39Z

shared/otel-core/src/otel/api/context/contextManager.ts

+ * @since 4.0.0
 */
-export function createContextManager(parentContext?: IOTelContext): IOTelContextManager {
+export function createContextManager(config?: IContextManagerConfig): IOTelContextManager {


As this is our distro, I'd pass the core sdk config (which is already dynamic)

MSNev · 2026-03-26T23:45:42Z

shared/otel-core/src/otel/api/context/contextManager.ts

+    // Register for config changes — save the returned IUnloadHook
+    // Only set up dynamic config watcher when config is provided to avoid per-instance overhead
+    if (config) {
+        let _configUnload = createDynamicConfig(_cfg).watch(function () {


not sure about this, it probably should just be applying defaults (if we want to split the defaults to each component) and then this would just use the onConfigChange helper as it's a single function (some minified) vs the watch not getting minified.

MSNev · 2026-03-26T23:50:29Z

shared/otel-core/src/otel/api/context/contextManager.ts

+    if (config) {
+        let _configUnload = createDynamicConfig(_cfg).watch(function () {
+            _handlers = _cfg.errorHandlers || {};
+            parentContext = _cfg.parentContext;


Question: For the "parentContext" do we or is it valid to change the parent context after construction -- it really just seems to be (currently) the "fallback" for when there is no "active" or when the current context is disabled?

Based on that I'd almost consider changing this to

createContextManager(config?: IOTelConfig, parentContext?: IOTelContextManager): IOTelContextManager

MSNev · 2026-03-26T23:54:47Z

shared/otel-core/src/otel/api/trace/span.ts

    function _handleIsEnded(operation: string, extraMsg?: string): boolean {
        if (isEnded) {
-            handleSpanError(errorHandlers, "Span {traceID: " + spanContext.traceId + ", spanId: " + spanContext.spanId + "} has ended - operation [" + operation + "] unsuccessful" + (extraMsg ? (" - " + extraMsg) : STR_EMPTY) + ".", spanName);
+            handleSpanError(_errorHandlers(), "Span {traceID: " + spanContext.traceId + ", spanId: " + spanContext.spanId + "} has ended - operation [" + operation + "] unsuccessful" + (extraMsg ? (" - " + extraMsg) : STR_EMPTY) + ".", spanName);


Maybe we should consider updating the handleErrors.ts so they could be passed either the IOTelErrorHandlers or IOTelWebSdkConfig and then we can put the "dereferencing" from the config to the configured error handlers in a single location instead of having the helper dereferencing in all components

MSNev · 2026-03-26T23:56:59Z

shared/otel-core/src/otel/api/trace/tracerProvider.ts

+    let _host = config.host;
+
+    // Register for config changes — save the returned IUnloadHook
+    let _configUnload = createDynamicConfig(config).watch(function () {


It should use onConfigChange and then it removes the need for "getting" the host first (calling createDynamicConfig by default uses the passed config as a template, so it's possible that the host object returned on line 65 is not the same object on line 70.

MSNev · 2026-03-27T00:00:54Z

shared/otel-core/src/otel/api/trace/tracerProvider.ts

-                tracers[tracerKey] = _createTracer(host);
+            if (_isShutdown) {
+                handleWarn(_handlers, "A shutdown TracerProvider cannot provide a Tracer");
+                return _NOOP_TRACER;


Noooo!!! ;-)

I would prefer that this just return null, and only if we "really" need to support a tracer that does nothing we can add a flag to the _createTracer so it does nothing (as required).

I think the way I did this in the 3.4.0 code base was that it always returns a tracer (which is basically the host itself) and during the "end" processing it then return a span that is not recording etc.

MSNev · 2026-03-27T00:04:24Z

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts

+ */
 export function createLoggerProvider(
-    config: IOTelLoggerProviderConfig = {}
+    config: IOTelLoggerProviderConfig


same comment as the context pass the IOTelWebSdkConfig instance and use the applyDefaults() to populate the log provider.

Basically, do we need to support the end-users creating their own LoggerProvider? As I would think we just want them to use ours.

MSNev · 2026-03-27T00:04:56Z

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts

-    const handlers: IOTelErrorHandlers = {};
+    // Register for config changes — save the returned IUnloadHook
+    // Updates both local cached values and sharedState so dynamic config changes propagate
+    let _configUnload = createDynamicConfig(config).watch(function () {


same as previous comments

MSNev · 2026-03-27T00:06:30Z

shared/otel-core/src/otel/sdk/OTelWebSdk.ts

-    let _loggerProvider = createLoggerProvider({
+    // Create a shared config object for the logger provider that gets updated
+    // when the SDK config changes, so the provider picks up new values.
+    let _loggerProviderConfig = {


I think that this should just be part of the shared Sdk config instead of as an internal reference

Phase 0 implementation

3340a5b

hectorhdzg requested a review from a team as a code owner March 20, 2026 23:53

Copilot AI review requested due to automatic review settings March 20, 2026 23:53

Copilot started reviewing on behalf of hectorhdzg March 20, 2026 23:53 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

JacksonWeber reviewed Mar 21, 2026

View reviewed changes

shared/otel-core/src/otel/sdk/OTelLoggerProvider.ts Outdated Show resolved Hide resolved

hectorhdzg added 2 commits March 24, 2026 14:27

Addressing comments

e2c61b2

Uodate

61c2904

MSNev reviewed Mar 26, 2026

View reviewed changes

MSNev reviewed Mar 27, 2026

View reviewed changes

Conversation

hectorhdzg commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants