fix: Use ROOT_CONTEXT instead active context as default in extract

[Flarna]

Which problem is this PR solving?

Avoid leaking current context into extracted context on default by using ROOT_CONTEXT as default for extract.

Short description of the changes

If someone wants to extract something it's usually not wanted to leak existing information into the extracted result.
e.g. consider an incoming HTTP instrumentation extracting a W3C tag. if active context is used it may get the current active span instead that once extracted from W3C tag or undefined.

In general I would prefer to have no default at all for context but this would require to remove also the default argument for getter which is fine as it is.

[codecov]

Codecov Report

Merging #1714 (e02ba93) into master (91612c4) will increase coverage by 0.17%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1714      +/-   ##
==========================================
+ Coverage   91.35%   91.53%   +0.17%     
==========================================
  Files         165       71      -94     
  Lines        5138     1913    -3225     
  Branches     1056      400     -656     
==========================================
- Hits         4694     1751    -2943     
+ Misses        444      162     -282     

Impacted Files	Coverage Δ
packages/opentelemetry-api/src/api/propagation.ts	57.57% <50.00%> (+1.32%)	⬆️
...s/opentelemetry-core/src/platform/node/sdk-info.ts	0.00% <0.00%> (-100.00%)	⬇️
.../opentelemetry-api/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...pentelemetry-core/src/platform/node/performance.ts	0.00% <0.00%> (-100.00%)	⬇️
...ckages/opentelemetry-core/src/common/NoopLogger.ts	40.00% <0.00%> (-20.00%)	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	80.43% <0.00%> (-19.57%)	⬇️
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts
...ges/opentelemetry-metrics/src/SumObserverMetric.ts
...ntelemetry-metrics/src/export/aggregators/index.ts
...s/opentelemetry-plugin-grpc-js/src/server/index.ts
... and 90 more

[Flarna]

Build fail seems to be unrelated: npm ERR! enoent ENOENT: no such file or directory, lstat '/root/.npm/_locks/staging-c354a3fda29e7624.lock'

[dyladan]

Build fail seems to be unrelated: npm ERR! enoent ENOENT: no such file or directory, lstat '/root/.npm/_locks/staging-c354a3fda29e7624.lock'

Retriggered the build

[dyladan]

In general I would prefer to have no default at all for context but this would require to remove also the default argument for getter which is fine as it is.

Or reorder the arguments. If we want to do something like that now is the time. After GA it will be impossible.

[Flarna]

I'm fine with changing the order.

But for consistency I would also change inject. And I propose to use context, carrier, setter like in TextMapPropagator.

[Flarna]

Has anyone else an opinion regarding this? I would like to avoid changing a lot without a chance that it gets merged.

[vmarchaud]

Has anyone else an opinion regarding this? I would like to avoid changing a lot without a chance that it gets merged.

I'm fine if we can do the same order as the Propagator's inject/extract

[dyladan]

I am also ok with the ordering change. @obecny ?

[obecny]

context is now optional (as last argument) if we move it to 1st place you will always have to provide it.
Is this what you want to change or something else ?

[dyladan]

context is now optional (as last argument) if we move it to 1st place you will always have to provide it.
Is this what you want to change or something else ?

That's the whole intention of the change. The issue is if you do this:

context = propagation.extract(headers);
span = getParentSpan(context);
// is span extracted from headers, or did it already exist in the active context?

You can solve this by using Context.ROOT_CONTEXT as the context for the extraction, but @Flarna is making the argument that when you're doing extract you should always be explicit about which context you're extracting into.

[Flarna]

Using the active context in extract is error prone in my opinion so moving to ROOT_CONTEXT is the minimal step to fix this.

But in general I think that requesting the users to be more explicit is better here. I will create a competing PR with extract/inject adapted tomorrow.

[obecny]

it sounds reasonable go for it :)

[Flarna]

close in favor of #1734