Prototype for sessions as entities#1
Conversation
| // Rebind on session rotation | ||
| sessionManager.addObserver({ | ||
| onSessionStarted(session) { | ||
| loggerProvider.setEntity(createSessionEntity(session.id)); |
There was a problem hiding this comment.
This shows user code updating the global logger provider with a new session entity.
| * route telemetry through the new entity-bound provider. | ||
| */ | ||
| setEntity(entity: Entity): void { | ||
| this._currentProvider = this.forEntity(entity); |
There was a problem hiding this comment.
If we are replacing the current provider I wonder if its necessary to shut it down 1st 🤔
There was a problem hiding this comment.
Pull request overview
Prototype implementation for modeling browser sessions as Entity-bound Resources, aligned with the proposed forEntity() provider API, plus a runnable example demonstrating dynamic session rebinding for logs.
Changes:
- Export new entity utilities from
@opentelemetry/sdk-browser(Entity types, session entity creator, resource merge helper, entity-aware logger provider). - Introduce
EntityAwareLoggerProviderandmergeEntityIntoResource()to bind (and rebind) log telemetry to a session entity via Resource attributes. - Add a new
examples/session-entitywebpack demo and update Biome config to ignore generatedbundle.js.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-browser/src/index.ts | Re-exports new entity/session APIs from the SDK package entrypoint. |
| packages/sdk-browser/src/entity/Entity.ts | Defines the Entity interface used by the prototype. |
| packages/sdk-browser/src/entity/createSessionEntity.ts | Adds helper to create a browser.session entity with session.id. |
| packages/sdk-browser/src/entity/mergeEntityIntoResource.ts | Merges entity attributes into a Resource to model entity-bound providers. |
| packages/sdk-browser/src/entity/EntityAwareLoggerProvider.ts | Adds an entity-rebindable logger provider using dynamic delegation. |
| packages/sdk-browser/src/configuration.ts | Switches logs session tracking to use EntityAwareLoggerProvider when enabled. |
| examples/session-entity/* | Adds a runnable demo showcasing rebinding behavior across session rotation. |
| biome.jsonc | Ignores generated bundle.js. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "bundler", | ||
| "allowImportingTsExtensions": false, |
There was a problem hiding this comment.
tsconfig.json disables allowImportingTsExtensions, but this example imports local modules using ".ts" extensions (e.g., "./otel-config.ts"). This will fail type-checking with TS5097 under common TypeScript configurations. Either set allowImportingTsExtensions to true (and ensure the bundler supports it) or remove the ".ts" extensions from local imports.
| "allowImportingTsExtensions": false, | |
| "allowImportingTsExtensions": true, |
| // For logs: use EntityAwareLoggerProvider when session tracking is enabled. | ||
| // This models the session as an Entity on the Resource (per the Entity Provider OTEP) | ||
| // instead of injecting session.id as an attribute via a processor. | ||
| let loggerProvider: LoggerProvider | EntityAwareLoggerProvider; | ||
| if (sessionManager) { |
There was a problem hiding this comment.
configureBrowserSDK has existing Vitest coverage (configuration.test.ts), but the newly introduced session-tracking log path (EntityAwareLoggerProvider + entity binding/rebinding) isn't tested. Adding a test that enables session tracking and asserts logs include session.id on the Resource (and that it changes after rotation) would help prevent regressions.
| entityLoggerProvider.setEntity(createSessionEntity(session.id)); | ||
| }, | ||
| onSessionEnded() { | ||
| entityLoggerProvider.forceFlush(); |
There was a problem hiding this comment.
forceFlush() returns a Promise, but it's called without awaiting or handling rejection. This can cause unhandled promise rejections and also race with shutdown (sessionManager.shutdown() may trigger this observer right before loggerProvider.shutdown()). Consider using void ...forceFlush().catch(...) or restructuring so flush is awaited by an async caller.
| entityLoggerProvider.forceFlush(); | |
| void entityLoggerProvider.forceFlush().catch(() => { | |
| // Intentionally ignore errors to avoid unhandled rejections on session end. | |
| }); |
| const entityAttributes: Record<string, string | number | boolean> = { | ||
| ...entity.identifier, | ||
| }; | ||
|
|
||
| if (entity.attributes) { | ||
| for (const [key, value] of Object.entries(entity.attributes)) { | ||
| // Descriptive attributes don't override existing values | ||
| if (!(key in entityAttributes)) { |
There was a problem hiding this comment.
Descriptive entity attributes are supposed to be included without overriding existing base Resource attributes, but this loop only prevents overriding identifier attributes (entityAttributes). Because the merged Resource is later created and merged into baseResource, descriptive keys that already exist on baseResource will still override. Filter descriptive keys against baseResource's existing attributes (or merge in two steps) to match the documented behavior.
| const entityAttributes: Record<string, string | number | boolean> = { | |
| ...entity.identifier, | |
| }; | |
| if (entity.attributes) { | |
| for (const [key, value] of Object.entries(entity.attributes)) { | |
| // Descriptive attributes don't override existing values | |
| if (!(key in entityAttributes)) { | |
| const baseAttributes = baseResource.attributes ?? {}; | |
| const entityAttributes: Record<string, string | number | boolean> = { | |
| ...entity.identifier, | |
| }; | |
| if (entity.attributes) { | |
| for (const [key, value] of Object.entries(entity.attributes)) { | |
| // Descriptive attributes don't override existing values or base resource attributes | |
| if (!(key in entityAttributes) && !(key in baseAttributes)) { |
| resource ?? | ||
| ({ | ||
| attributes: {}, | ||
| merge: (r: Resource | null) => r ?? this._baseResource, | ||
| getRawAttributes: () => [], |
There was a problem hiding this comment.
This fallback Resource is a hand-rolled stub and likely does not satisfy the full Resource contract (e.g., missing standard methods and potentially different merge semantics). Since EntityAwareLoggerProvider is exported publicly, construct a real empty Resource instead (e.g., resourceFromAttributes({}) or Resource.empty() if available) rather than using a type assertion.
This is intended as a prototype of modeling browser sessions using entities based on this proposal:
OTEP: Allow multiple Resources in an SDK by jsuereth · Pull Request #4665 · open-telemetry/opentelemetry-specification.
The proposal introduces a
forEntity()method on providers. I think this works when a single module (e.g. single instrumentation) is in control of the entity-bound provider.However, the session entity should be applied to all telemetry. When any instrumentation (and user code) emits a signal, it should include the session attributes. And when a session rotates, this needs to propagate to all.
This prototype includes an
EntityAwareLoggerProviderthat delegates to an internal LoggerProvider and allows updating the entity via thesetEntity()method. Internally, the delegate LoggerProvider is constructed by using theforEntity()method as described in the proposal.