Remove suppressInstrumentation

[dyladan]

Spec PR ongoing at the time of this issue creation open-telemetry/opentelemetry-specification#1653

While this functionality is not in the spec, we need to remove it from the API. We can create a package in SDK for API extensions, or include it in the instrumentation package, if we want to expose this functionality to instrumentations.

[blumamir]

There are cases where suppressInstrumentation is used not in instrumentations context. We as a vendor use it in our sdk to suppress traces for our backend services communication. For example: calls to config service which we don't want to exhaust user attention with.

I would prefer for it to reside in api extension package, so users wouldn't need to take dependency on instrumentation package when they only need suppressInstrumentation util.

[vmarchaud]

While this functionality is not in the spec, we need to remove it from the API. We can create a package in SDK for API extensions, or include it in the instrumentation package, if we want to expose this functionality to instrumentations.

So the plan is to make the effort to split it out right now to release the GA, wait for the spec to ev entually add it to the spec and merge it back into the API with a minor release ?

[obecny]

While this functionality is not in the spec, we need to remove it from the API. We can create a package in SDK for API extensions, or include it in the instrumentation package, if we want to expose this functionality to instrumentations.

So the plan is to make the effort to split it out right now to release the GA, wait for the spec to eventually add it to the spec and merge it back into the API with a minor release ?

this sounds awkward to me, this will be spec'ed sooner or later anyway, we have this functionality already in place seems people are using this, not sure if this is a right move to remove it now to just add it again very soon

[Flarna]

I guess the problem is that spec may require it different.
=> Removing now and adding later would be semver minor
=> keeping now and changing later would be semver major

[vmarchaud]

I guess the problem is that spec may require it different.
=> Removing now and adding later would be semver minor
=> keeping now and changing later would be semver major

I totally understand this, but what i don't understand is that they requires us to make the chance now so spec reviewers can take their time to add it to the API. I feel if we just do the change it will take weeks/months to get this into the API even though every language have this concept.
Why not just wait for specs to do our API GA ? If they just ask for few weeks to think about it, why not just give them ?

[Flarna]

So you mean keep it as is and delay GA of JS API until spec is finished regarding this?

[vmarchaud]

So you mean keep it as is and delay GA of JS API until spec is finished regarding this?

Yeah

[dyladan]

We have been told specifically by the TC to remove it. There is no guarantee it will be spec'd in its current state and the spec PR has recieved quite a bit of pushback.

They suggested we create an API extension package for functionality like this which we want to provide to users but is not in the spec.

[dyladan]

While this functionality is not in the spec, we need to remove it from the API. We can create a package in SDK for API extensions, or include it in the instrumentation package, if we want to expose this functionality to instrumentations.

So the plan is to make the effort to split it out right now to release the GA, wait for the spec to ev entually add it to the spec and merge it back into the API with a minor release ?

Yes, if it ever is added to the spec, but that is not certain. There were also other solutions proposed to solve the same use-cases.

Other languages have this concept but they actually only have it in their SDK. They don't expose it to instrumentation at all.

[vmarchaud]

Well i think implementing into SDK ourselves seems the most coherent things to do. What about:

• implementing the context methods inside the instrumentation package so we can instrumentation can use it if they want to.
• have the tracing sdk use the same context key (with Symbol.for) to lookup if the tracing is disabled or not

I suggest this because having yet another package that we need to maintain (and for contributor to understand) seems complex, WDYT ?

[Flarna]

I think instrumentation package is a good place for this. Every instrumentation depends already on it.

[dyladan]

The tracing package would then need to depend on the instrumentation package (which creates a circular dependency). Seems quite unexpected and cumbersome to me. I would implement it in core and allow instrumentations to depend on core to use it. This makes it clear that it is an implementation detail of our SDK and if the instrumentation is used with another SDK then it is not guaranteed to work.

The other option is to create an api-ext package which contains unofficial features like this.

edit: I see you don't have it depend, but have both packages just use the same key with Symbol.for. This seems ok but we would need to make sure to add a comment in the code or somewhere that the symbol is duplicated and if it changes in one place it could break another.

edit: the more I think on this, the more I think the api-ext package is probably the right choice.

[obecny]

I would be in favour of adding extra package, not sure though where it should sit, but having extra package would be easiest way for the end user because the package will not depend on anything except api.
In case the ext / addons /contrib (or whatever the name will be) package might have the same name for functionalities should we consider using some prefixes to indicate it is not api. And in case the api will eventually have function under the same name to avoid any future conflicts ?

[dyladan]

I think it should probably live in open-telemetry/opentelemetry-js repo to make it clear that the extensions are not officially part of open-telemetry/opentelemetry-js-api and may not work with other SDKs.

[obecny]

we could potentially include there some few helpers that we can't add under api official package ......