Web SDK

[martinkuba]

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]

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

[pkanal]

Suggested change

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

[pichlermarc]

Oops, looks like this is still here.

[github-actions]

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]

Suggested change

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

[AbhiPrasad]

Suggested change

	tracerConfig: tracerConfig,
	tracerConfig,

[AbhiPrasad]

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]

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]

Suggested change

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

[AbhiPrasad]

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]

Oops, looks like this is still here.

[pichlermarc]

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.

[AbhiPrasad]

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]

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?

I think there's there's some very distant idea to have something like what youre describing in the future (I'm not aware of any concrete proposal though, which I imagine would need to happen on the spec side to ensure interoperability with the current API). However, in its current state, the API only accepts singleton registration, so it would really only be called once.

That NodeSDK is a class is historical (initial decisions happened before I joined the project so I'm just speculating how this happened):

• it had a not-very-well-executed builder-patten-ish interface in the past which mostly duplicated the constructor, made the code very complex, and confused users (we've since removed that)
• one had to eventually await sdk.start(), because ResourceDetector#detect() was async.
  • We've since removed that limitation by introducing async resource attributes that will resolve in the export pipeline (Create sync resource with some attributes that resolve asynchronously #3460). This also eliminated the need to await in SDK Setup. This caused issues in CJS as there's no top-level await and SDK setup is supposed to finish before any user code is executed.

If I had to re-do @opentelemetry/sdk-node, I'd also change it to be a function.

[pichlermarc]

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]

I wonder if we can better structure this. Something like

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

[pichlermarc]

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]

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

[scheler]

@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.

[martinkuba]

Thank you everyone for your comments. I have moved this back to draft, as it is not clear that this is necessarily the right direction. I have summarized the current state in this doc. My plan is to continue working on a design and prototyping with others in the Client Instrumentation SIG. I will re-open this PR (or a new one) once it is more clear what path we want to pursue.

[lingyan]

@martinkuba Thanks for the effort! Do you mind sharing the bundle size (minified/compressed) of this web sdk (core trace support without any plugins) at the moment?

[github-actions]

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.