Opentelemetry-js: getExtractedSpanContext does not return correct result with multiple versions of @opentelemetry/core are installed

[blumamir]

Please answer these questions before submitting a bug report.

What version of OpenTelemetry are you using?

My tracer is v0.9.0, and a plugin is using v0.8.3

What version of Node are you using?

v13.11.0

What did you do?

My project has a tracer with v0.9.0, and is using a plugin with v0.8.3.

1. I extracted a propagated context with propagation.extract(...), under the hood, this operation calls _getGlobalPropagator which return the v0.9.0 propagator which then calls the setExtractedSpanContext v.0.9.0 function.
2. I called the getExtractedSpanContext function on the Context from 1, which uses the v0.8.3 version.

What did you expect to see?

I expected the getExtractedSpanContext function to return the extractedSpanContext which is set on the Context

What did you see instead?

The function falsly returns undefined.

Additional context

I think the issue is that Context.createKey is creating a regular Symbol, which means that every version in node_modules will have a unique symbol for that version, instead of creating it globally with Symbol.for(), like here

[dyladan]

Thanks for reporting this issue. Seems related to the issue https://github.com/open-telemetry/opentelemetry-js/issues/1370 reported earlier.

I can think of a few ways to fix this:

1. Change context keys to use Symbol.for so they are always the same key no matter what version of core is used
2. Move context key creation into the API, which is already guaranteed to be a "true global"
3. Check version when plugins are loaded to ensure the same version of core is used across the whole project

@obecny @mayurkale22 do you have opinions on this?

[obecny]

it's the same problem I reported and described here -> https://github.com/open-telemetry/opentelemetry-js/issues/1370.
Each of the plugin have it's own folder node_modules and will use the api from there instead of sharing the same api , core etc. So we should cut a release on contrib repo asap so that the plugins can use the same core / api instead of having its own node_modules. Implementing a mechanism which would prevent loading a plugin which has dependency on older version of api/core is something we should add to as separate PR. But for now the easiest solution is to simply make a new release for contrib plugins without waiting for anything

[dyladan]

Just made a PR to release contrib, but this issue will still exist.

API does not have this issue since it uses the global utils to ensure a single global API instance is used, even if multiple api instances of varying versions exist in the node_modules structure.

[blumamir]

Thanks @dyladan @obecny
In my opinion option 1 is a good fix in addition to the release of contrib repo.
I encountered the issue in my plugin which is not in contrib.
I can create a PR to change Symbol().. to Symbol.for(...)

[dyladan]

I agree, option 1 is also the option with the least impact. (no api or behavior change)

[obecny]

If Symbol.for is using runtime-wide symbol in theory we might have some name collisions and we end up to have some prefixed naming (this might be necessary anyway). Having that in mind, I would be in favour of creating api methods to create those symbols (option 2) as the only prefered way (maybe some lint rule too) so that we can add some prefix automatically. This way we if for any reason we will have to change the prefix in future (newer api cannot be compatible backwards for example) we will have a way of doing that without "polluting" the previous version

[blumamir]

@obecny the current Symbol text is 'OpenTelemetry Context Key EXTRACTED_SPAN_CONTEXT', which already contains few prefixes, and is only used as a key on the Context object. I believe runtime-wide name collisions are not relevant in this case.
In addition, the project already implements this pattern in the API package

I prefer option 1 as it is simple, but moving the symbols to API package is also fine. Do you want to take this issue and create a PR? I can also do it myself

[dyladan]

There is some discussion in the spec about requiring the API package to propagate baggage/correlations by default, because end-user applications will depend on it for behavior so it cannot be no-op. Because of this, the API will need to access context properties.

This leads me to think we should move the context symbols into the API package anyways, which solves this issue without introducing the runtime-wide Symbol.for symbols.

WDYT

[blumamir]

There will still be Symbol.for, only it will be in the API package, no?
Like these (from /packages/opentelemetry-api/src/api/global-utils.ts):

export const GLOBAL_CONTEXT_MANAGER_API_KEY = Symbol.for(
  'io.opentelemetry.js.api.context'
);
export const GLOBAL_METRICS_API_KEY = Symbol.for(
  'io.opentelemetry.js.api.metrics'
);
export const GLOBAL_PROPAGATION_API_KEY = Symbol.for(
  'io.opentelemetry.js.api.propagation'
);
export const GLOBAL_TRACE_API_KEY = Symbol.for('io.opentelemetry.js.api.trace');

[dyladan]

Actually after looking into it a little more, createKey doesn't need to be moved into the API, just called by the API.

There will still be Symbol.for, only it will be in the API package, no?

No. The spec states that context keys are generated by calling an API, and that calling the API twice _never_ returns the same key, even if it is called with the same input.

I am proposing that the trace API exposes new methods like api.trace.getActiveSpan(context). The context symbols become private members of the TraceAPI instance. Since we already guarantee there is a single global trace api per process, this ensures there is no confusion of context keys.

[dyladan]

I will open a PR to illustrate

[dyladan]

In trying to open this PR, it is becoming much more complex than I thought. In the spec, the wording is this:

Multiple calls to CreateKey with the same name SHOULD NOT return the same value unless language constraints dictate otherwise

I think we have such a language restriction that will allow us to have an exception here. By far the simplest way to solve this problem is to use Symbol.for

[blumamir]

What is the intention?
Why would we want to call CreateKey more than once? and if called multiple times, why return different values?

[dyladan]

The intention is to make it impossible for keys to collide. I guess some languages use context propagation mechanisms that are well-known and used by many other things.

[blumamir]

Not sure I fully understand, but I'm in favor of the Symbol.for(...) option.
Maybe we can ask there for an example of why CreateKey should return different keys for the same values? so we can estimate if it's relevant to JS implementation

[adamegyed]

Just adding to this: the fix implemented here also affects the other methods in the global context api: getParentSpanContext and getActiveSpanContext when used from a separate import. Previously, they would be unable to detect the global context's SpanContext due to the uniquely generated symbols.