Conversation
There was a problem hiding this comment.
Pull request overview
Implements the Phase 1 “SDK Foundation” for the OTel Web SDK by introducing a new factory-based createOTelWebSdk() entry point and associated interfaces, while removing the legacy OTelSdk dynamicProto-based class and wiring in unit tests.
Changes:
- Added
createOTelWebSdk()factory and newIOTelWebSdk/IOTelWebSdkConfigpublic interfaces. - Removed legacy
OTelSdk+ related interfaces/exports and updatedshared/otel-corepublic exports accordingly. - Fixed
createContext().setValue()to return a new context instance and added extensive unit coverage for the new SDK.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/otel-core/src/utils/DataCacheHelper.ts | Changes cache namespace versioning behavior (currently hardcodes a version string). |
| shared/otel-core/src/otel/sdk/OTelWebSdk.ts | Adds the new SDK factory implementation (tracer + logger access, lifecycle APIs). |
| shared/otel-core/src/otel/sdk/OTelSdk.ts | Removes the legacy dynamicProto-based SDK class. |
| shared/otel-core/src/otel/api/context/context.ts | Fixes context immutability behavior for setValue(). |
| shared/otel-core/src/interfaces/otel/trace/IOTelTracerCtx.ts | Updates interface documentation wording. |
| shared/otel-core/src/interfaces/otel/config/IOTelWebSdkConfig.ts | Introduces SDK configuration interface for dependency injection. |
| shared/otel-core/src/interfaces/otel/IOTelWebSdk.ts | Introduces the main SDK interface (tracer/logger/lifecycle/config). |
| shared/otel-core/src/interfaces/otel/IOTelSdkCtx.ts | Removes legacy SDK context interface. |
| shared/otel-core/src/interfaces/otel/IOTelSdk.ts | Removes legacy SDK interface. |
| shared/otel-core/src/index.ts | Updates public exports to expose the new SDK API and remove the old one. |
| shared/otel-core/Tests/Unit/src/sdk/OTelWebSdk.Tests.ts | Adds unit tests covering construction, tracing, logging, flush/shutdown, and config. |
| shared/otel-core/Tests/Unit/src/index.tests.ts | Registers the new OTelWebSdk test suite. |
| if (!config.performanceNow) { | ||
| handleError(_handlers, "createOTelWebSdk: performanceNow must be provided"); | ||
| } |
There was a problem hiding this comment.
The code validates performanceNow as a required config dependency, but the value is never used anywhere in OTelWebSdk.ts. Either wire performanceNow into span/log timing (or wherever it’s intended to be used) or make it optional so consumers aren’t forced to pass a dead field.
| if (!config.performanceNow) { | |
| handleError(_handlers, "createOTelWebSdk: performanceNow must be provided"); | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
shared/otel-core/src/interfaces/otel/config/IOTelWebSdkConfig.ts
Outdated
Show resolved
Hide resolved
| _self.forceFlush = function (): IPromise<void> { | ||
| if (_isShutdown) { | ||
| handleWarn(_handlers, "Cannot force flush a shutdown OTelWebSdk"); | ||
| return createSyncPromise(function (resolve) { |
There was a problem hiding this comment.
Small code improvement (minification) there is a createSyncResolvedPromise which does this already
| let spanAttributes = attributes; | ||
| if (isRecording && samplingResult.attributes) { | ||
| // Merge: user attributes take precedence, sampler attributes fill gaps | ||
| spanAttributes = {}; |
There was a problem hiding this comment.
We should use the AttributeContainer for this to avoid copying / enumerating all of the attributes unless needed.
| let activeCtx = ctx || _getActiveContext(); | ||
| let contextWithSpan = setContextSpan(activeCtx, span); | ||
|
|
||
| return _contextManager.with(contextWithSpan, function () { |
There was a problem hiding this comment.
likewise this this we have "useSpan" and "withSpan" helpers for the same reason.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
MSNev
left a comment
MSNev
left a comment
There was a problem hiding this comment.
There are some possible minor issues, but this is a good start for us to build upon and find the stuff we don't know yet :-)
Phase 1: SDK Foundation (Critical)
Component Location Description
createOTelWebSdk() src/otel/sdk/ Main SDK entry point factory
IOTelWebSdkConfig src/interfaces/otel/config/ SDK configuration interface
Deprecate OTelSdk class src/otel/sdk/OTelSdk.ts Remove DynamicProto usage
Deliverable: Functional SDK that can be instantiated with trace+log providers.