Web SDK #4325

martinkuba · 2023-11-28T01:09:16Z

Related issue #4702

Short description of the changes

This introduces a Web SDK, similar to the Node SDK package. This should simplify installation and configuration for users. Currently, it includes only tracing, but in the future it will include event/log SDK as well.

I have also included the SessionId span processor, which adds the session.id attribute to all spans. This is similar to what the Android SDK has already implemented. In the future there will likely be other processors added here for additional attributes such as the page URL, visitor ID etc.

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

codecov · 2023-11-28T01:13:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.62%. Comparing base (ecc88a3) to head (8074d25).
Report is 45 commits behind head on main.

❗ Current head 8074d25 differs from pull request most recent head d2eabdd

Please upload reports for the commit d2eabdd to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4325      +/-   ##
==========================================
+ Coverage   91.04%   92.62%   +1.58%     
==========================================
  Files          89      296     +207     
  Lines        1954     8300    +6346     
  Branches      416     1717    +1301     
==========================================
+ Hits         1779     7688    +5909     
- Misses        175      612     +437

see 264 files with indirect coverage changes

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts

pkanal · 2023-11-28T17:26:04Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ private _disabled?: boolean;
+
+ /**
+ * Create a new NodeJS SDK instance


Suggested change

* Create a new NodeJS SDK instance

* Create a new Web SDK instance

Oops, looks like this is still here.

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

experimental/packages/opentelemetry-sdk-web/README.md

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts

github-actions · 2024-04-15T06:34:07Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

AbhiPrasad

I left some feedback to try to reduce the bundle size impact of this sdk.

Although this does hurt readability in some areas, I believe it's important that OTEL instrumentation attempts to stay as bundle efficient as possible. We can always rely on a robust test suite to make sure intent is shared.

Feel free to disregard the review comments which you don't agree with. Thanks!

AbhiPrasad · 2024-06-05T15:58:33Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ let instrumentations: InstrumentationOption[] = [];
+ if (configuration.instrumentations) {
+ instrumentations = configuration.instrumentations;
+ }
+ this._instrumentations = instrumentations;


Suggested change

let instrumentations: InstrumentationOption[] = [];

if (configuration.instrumentations) {

instrumentations = configuration.instrumentations;

}

this._instrumentations = instrumentations;

this._instrumentations = configuration.instrumentations ? configuration.instrumentations : [];

AbhiPrasad · 2024-06-05T15:58:46Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ ];
+
+ this._tracerProviderConfig = {
+ tracerConfig: tracerConfig,


Suggested change

tracerConfig: tracerConfig,

tracerConfig,

AbhiPrasad · 2024-06-05T16:00:30Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ if (configuration.spanProcessors || configuration.traceExporter) {
+ const tracerConfig: WebTracerConfig = {};
+
+ if (configuration.sampler) {


although this hurts readability, if we destructure configuration into individual properties we save on bundle size. Currently obj properties like configuration.sampler or configuration.spanLimits cannot be minified (therefore you'll end up with X.sampler multiple times in the bundle).

Unfortunately I've also found that obj properties don't gzip very well too 😬 - so the repeated strings aren't valuable there either.

ref: https://blog.sentry.io/performance-impact-of-generated-javascript/#alias-object-keys-to-local-variables-to-enable-minification

AbhiPrasad · 2024-06-05T16:13:19Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ if (this._autoDetectResources) {
+ const internalConfig: ResourceDetectionConfig = {
+ detectors: this._resourceDetectors,
+ };
+ this._resource = this._resource.merge(
+ detectResourcesSync(internalConfig)
+ );
+ }
+
+ this._resource =
+ this._serviceName === undefined
+ ? this._resource
+ : this._resource.merge(
+ new Resource({
+ [SEMRESATTRS_SERVICE_NAME]: this._serviceName,
+ })
+ );


Suggested change

if (this._autoDetectResources) {

const internalConfig: ResourceDetectionConfig = {

detectors: this._resourceDetectors,

};

this._resource = this._resource.merge(

detectResourcesSync(internalConfig)

);

}

this._resource =

this._serviceName === undefined

? this._resource

: this._resource.merge(

new Resource({

[SEMRESATTRS_SERVICE_NAME]: this._serviceName,

})

);

if (this._autoDetectResources) {

this._resource = this._resource.merge(

detectResourcesSync({ detectors: this._resourceDetectors })

);

}

if (this._serviceName !== undefined) {

this._resource = this._resource.merge(

new Resource({

[SEMRESATTRS_SERVICE_NAME]: this._serviceName,

})

);

}

AbhiPrasad · 2024-06-05T16:14:02Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+
+ if (this._tracerProviderConfig) {
+ const tracerProvider = new WebTracerProvider({
+ ...this._tracerProviderConfig?.tracerConfig,


Suggested change

...this._tracerProviderConfig?.tracerConfig,

...this._tracerProviderConfig.tracerConfig,

AbhiPrasad · 2024-06-05T16:14:15Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ contextManager: this._tracerProviderConfig?.contextManager,
+ propagator: this._tracerProviderConfig?.textMapPropagator,


Suggested change

contextManager: this._tracerProviderConfig?.contextManager,

propagator: this._tracerProviderConfig?.textMapPropagator,

contextManager: this._tracerProviderConfig.contextManager,

propagator: this._tracerProviderConfig.textMapPropagator,

pichlermarc

There are a few things I worry about:

Making WebSDK resemble NodeSDK

I think we can make a simple setup a function and simplify things this way.
From what I know NodeSDK was added a long time ago (3+ years ago) and it's more-or-less stuck now the way it is because "it's always been this way". We should not repeat the same mistakes with the WebSDK if we want to add it, and should consider a more ergonomic, easier to understand interface.

Managing scope

With NodeSDK we get constant requests for adding features. Adding features here would bloat up user's bundles once included. I think we should add some kind of guide of what is allowed in here and what we don't want to add, with reasoning as to why this is. This way we can more easily manage these requests in the future.

Package stability

With Events and Logs SDKs/APIs not being close to becoming stable, I'm worried that by building on top of these packages we introduce another package that will become "de-facto" stable. (little to no changes to it's public API over long periods of time that users start considering it the same as a 1.x package). I think we should formulate some plan how we want to ensure that this package will become stable eventually.

Production use

I'm still kind of torn on whether the package would improve the situation. The way I understand this package is that it is intended as a quick start, but I am very confident that this package will be used in production and may soon start to be considered "the only way" to set up the SDK, effectively hiding the fact that components can be set up manually too.

When done right, manual SDK setup is actually quite small in size - the difficulty comes from doing it in the correct order. I wonder if we can address these issues with better documentation on manual setup, or by improving the way setup works directly in the SDK packages. This would also allow users not to include packages they don't need (only depending on Logs/Events when that's the only thing they need).

cc @open-telemetry/javascript-maintainers would appreciate to hear your comments.

pichlermarc · 2024-06-25T08:51:53Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ private _disabled?: boolean;
+
+ /**
+ * Create a new NodeJS SDK instance


Oops, looks like this is still here.

pichlermarc · 2024-06-25T08:57:27Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+import { EventLoggerProvider } from '@opentelemetry/sdk-events';
+
+/** This class represents everything needed to register a fully configured OpenTelemetry Web SDK */
+export class WebSDK {


I wonder if this needs to be a class at all. Maybe it's better to have a setupWebSDK() (or similar) function be the only thing that's registered?

We can return the shutdown from setupWebSDK(). I'd probably also reduce the amount of code needed by a whole lot. IMO we should not copy NodeSDK as it's far from perfect.

there will def be bundle size benefits from using a function instead of a class, but depending on how it is designed the closure can be less memory efficient that a separate variable + constructor instantiation in the class if there are multiple instances created.

Do we see scenarios where you'll get multiple WebSDK instances on a page (micro-frontends) as a common pattern? Or is this really meant to be a singleton for 99% of cases?

pichlermarc · 2024-06-25T09:01:43Z

experimental/packages/opentelemetry-sdk-web/src/sdk.ts

+ private _instrumentations: Instrumentation[];
+
+ private _resource: IResource;
+ private _resourceDetectors: Array<Detector | DetectorSync>;


If possible, I'd only accept DetectorSync. The old resource detector interface has been deprecated for a long time.

Suggested change

private _resourceDetectors: Array<Detector | DetectorSync>;

private _resourceDetectors: Array<DetectorSync>;

pichlermarc · 2024-06-25T09:08:37Z

experimental/packages/opentelemetry-sdk-web/src/types.ts

+export interface WebSDKConfiguration {
+ autoDetectResources: boolean;
+ contextManager: ContextManager;
+ textMapPropagator: TextMapPropagator;
+ instrumentations: Instrumentation[];
+ resource: IResource;
+ resourceDetectors: Array<Detector | DetectorSync>;
+ sampler: Sampler;
+ serviceName?: string;
+ spanProcessors?: SpanProcessor[];
+ traceExporter: SpanExporter;
+ spanLimits: SpanLimits;
+ idGenerator: IdGenerator;
+ eventsLogRecordProcessors?: LogRecordProcessor[];
+ eventsLogRecordExporter?: LogRecordExporter;
+}


I wonder if we can better structure this. Something like

{ trace: { sampler: Sampler; ... }; events: { eventsLogRecordExporter: LogRecordExporter; ... }; }

pichlermarc · 2024-06-25T09:16:14Z

experimental/packages/opentelemetry-sdk-web/README.md

+### Initialize the SDK
+
+Before any other module in your application is loaded, you must initialize the SDK.
+If you fail to initialize the SDK or initialize it too late, no-op implementations will be provided to any library which acquires a tracer or meter from the API.


Suggested change

If you fail to initialize the SDK or initialize it too late, no-op implementations will be provided to any library which acquires a tracer or meter from the API.

If you fail to initialize the SDK or initialize it too late, no-op implementations will be provided to any library which acquires a tracer or logger from the API.

No metrics here right now. 🙂

pichlermarc · 2024-06-25T09:17:44Z

experimental/packages/opentelemetry-sdk-web/src/index.ts

+export * as api from '@opentelemetry/api';
+export * as contextBase from '@opentelemetry/api';
+export * as core from '@opentelemetry/core';
+export * as resources from '@opentelemetry/resources';
+export * as tracing from '@opentelemetry/sdk-trace-base';
+export * from './sdk';
+export * from './types';


is there a reason why we re-export so many sdk packages here? 🤔

scheler · 2024-06-25T17:24:12Z

@pichlermarc @martinkuba

I'm still kind of torn on whether the package would improve the situation. The way I understand this package is that it is intended as a quick start, but I am very confident that this package will be used in production and may soon start to be considered "the only way" to set up the SDK, effectively hiding the fact that components can be set up manually too.

I think we should add some kind of guide of what is allowed in here and what we don't want to add, with reasoning as to why this is.

I think the above 2 arguments are effective when taken together. A quick start package will be very helpful for a majority of cases, but at the same time it is fine to be highly opinionated about what goes into it since it affects the majority and redirect enhancement requests on the web-sdk to manual setup.

t2t2 reviewed Nov 28, 2023

View reviewed changes

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts Outdated Show resolved Hide resolved

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts Outdated Show resolved Hide resolved

pkanal reviewed Nov 28, 2023

View reviewed changes

dyladan reviewed Dec 1, 2023

View reviewed changes

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts Outdated Show resolved Hide resolved

pkanal mentioned this pull request Dec 4, 2023

feat: ✨ Pull in base OTel WebSDK honeycombio/honeycomb-opentelemetry-web#3

Merged

drewcorlin1 reviewed Dec 24, 2023

View reviewed changes

experimental/packages/opentelemetry-sdk-web/src/sdk.ts Show resolved Hide resolved

scheler reviewed Feb 13, 2024

View reviewed changes

experimental/packages/opentelemetry-sdk-web/README.md Outdated Show resolved Hide resolved

experimental/packages/opentelemetry-sdk-web/README.md Show resolved Hide resolved

experimental/packages/opentelemetry-sdk-web/src/SessionIdSpanProcessor.ts Outdated Show resolved Hide resolved

github-actions bot added the stale label Apr 15, 2024

github-actions bot removed the stale label Apr 22, 2024

feat: add Web SDK package

c8828b1

martinkuba force-pushed the web-sdk branch from 8074d25 to c8828b1 Compare May 16, 2024 23:37

martinkuba added 3 commits May 16, 2024 16:44

add karma.worker config file

0fcba52

markdown lint

f1dada6

added changelog

3cd7a74

martinkuba marked this pull request as ready for review May 16, 2024 23:54

martinkuba requested a review from a team as a code owner May 16, 2024 23:54

updated example

5b6fbfc

AbhiPrasad reviewed Jun 5, 2024

View reviewed changes

martinkuba added 8 commits June 5, 2024 14:34

Merge branch 'main' into web-sdk

f658f4d

Merge remote-tracking branch 'origin/main' into web-sdk

68896a5

updated versions

0a9ecb3

switched InstrumentationOption to Instrumentation

5c63e90

Merge branch 'main' into web-sdk

50d6544

added configuring EventLoggerProvider

4c6b199

Merge branch 'main' into web-sdk

4a10055

lint

d2eabdd

pichlermarc reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web SDK #4325

Web SDK #4325

martinkuba commented Nov 28, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

pkanal Nov 28, 2023

pichlermarc Jun 25, 2024

github-actions bot commented Apr 15, 2024

AbhiPrasad left a comment

AbhiPrasad Jun 5, 2024

AbhiPrasad Jun 5, 2024

AbhiPrasad Jun 5, 2024 •

edited

Loading

AbhiPrasad Jun 5, 2024

AbhiPrasad Jun 5, 2024

AbhiPrasad Jun 5, 2024

pichlermarc left a comment •

edited

Loading

pichlermarc Jun 25, 2024

pichlermarc Jun 25, 2024

AbhiPrasad Jun 25, 2024

pichlermarc Jun 25, 2024

pichlermarc Jun 25, 2024 •

edited

Loading

pichlermarc Jun 25, 2024

pichlermarc Jun 25, 2024

scheler commented Jun 25, 2024

	* Create a new NodeJS SDK instance
	* Create a new Web SDK instance

	...this._tracerProviderConfig?.tracerConfig,
	...this._tracerProviderConfig.tracerConfig,

		contextManager: this._tracerProviderConfig?.contextManager,
		propagator: this._tracerProviderConfig?.textMapPropagator,

	private _resourceDetectors: Array<Detector \| DetectorSync>;
	private _resourceDetectors: Array<DetectorSync>;

	If you fail to initialize the SDK or initialize it too late, no-op implementations will be provided to any library which acquires a tracer or meter from the API.
	If you fail to initialize the SDK or initialize it too late, no-op implementations will be provided to any library which acquires a tracer or logger from the API.

Web SDK #4325

Are you sure you want to change the base?

Web SDK #4325

Conversation

martinkuba commented Nov 28, 2023 • edited Loading

Short description of the changes

Type of change

Checklist:

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 15, 2024

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheler commented Jun 25, 2024

martinkuba commented Nov 28, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

AbhiPrasad Jun 5, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading

pichlermarc Jun 25, 2024 •

edited

Loading