feat: prototype context attach/detach

[pichlermarc]

Important

While this PR is intended to show possible use of attach/detach functionality for tracingChannel instrumentation, as an alternative to proposal #6088, It does not feature a production ready implementation of it
It's intended as a means to demonstrate how I expect this could be used to accomplish that use case.

Which problem is this PR solving?

Proposal #6088 is to allow accessing the internal AsyncLocalStorage from the outside, for the purpose of binding it to a TracingChannel, so that context propagation works properly. While this is works in practice it has a few downsides:

• the internal AsyncLocalStorage is internal to avoid tampering, misuse can lead to catastrophic effects and can be hard to debug
• not every ContextManager is a AsyncLocalStorageContextManager, so many do not have an AsyncLocalStorage. As such, we also cannot add it to the @opentelemetry/api package. Further AsyncLocalStorage is a Node.js type, and the @opentelemetry/api and its types must remain compatible with the Browser

My counter-proposal is implementing the spec-defined attach() and detach() operations to achieve a similar effect. This works by manually attaching context when the TracingChannel emits its events, storing the state on the message, and detaching the context once the end message is received.

This implementation:

• is specification compliant (we don't add any additional operations that don't exist in the spec)
• is runtime agnostic (these operations may be implemented however it makes sense for the ContextManager)
• (possibly) could open up other use-cases that we currently did not think of

The downsides:

• slightly more boilerplate code (I think most of it can be wrapped in utility functions so this may be acceptable)
• similar to exposing AsyncLocalStorage, context attach/detach can have potentially catastrophic effects (however, I think it's easier to document that than with exposing AsyncLocalStorage and other language SIGs have been managing fine with this operation available)

You can find an example how tracing channel instrumentation would work at examples/tracing-channels.

Previous work on context attach/detach

• feat(context): add attach and detach methods to context API opentelemetry-js-api#123
• feat(api/experimental): add context attach/detach methods #4669

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.47%. Comparing base (819ebb2) to head (b9fbb3f).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
api/src/api/context.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6387      +/-   ##
==========================================
- Coverage   95.50%   95.47%   -0.03%     
==========================================
  Files         365      365              
  Lines       11606    11627      +21     
  Branches     2676     2683       +7     
==========================================
+ Hits        11084    11101      +17     
- Misses        522      526       +4     

Files with missing lines	Coverage Δ
...async-hooks/src/AsyncLocalStorageContextManager.ts	100.00% <100.00%> (ø)
api/src/api/context.ts	78.57% <0.00%> (-13.10%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dyladan]

this looks really good to me. I think it's well past time we implemented something like this

[dyladan]

Does attach/detach need to be optional here? I think it only needs to be optional in the type the SDK implements and we can do the null check before calling the SDK impl

[dyladan]

include @experimental?

[dyladan]

include @experimental?

[dyladan]

Maybe attach should return a disposable so that it can be used with the using directive https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/using

[dyladan]

Can't comment on files not in the diff so i'm leaving this here in order to make it a thread:

Did you choose not to implement in ZoneContextManager because it was impossible or just to make the PR easier and save time? I think we would do well to include a browser-compatible version of this if possible when the feature is released.

[dyladan]

symbol doesn't work because it can't key a WeakMap. I ended up using Context return here https://github.com/dynatrace-oss-contrib/opentelemetry-js/blob/zone-attach-detach/packages/opentelemetry-context-zone-peer-dep/src/ZoneContextManager.ts

[dyladan]

Any reason you chose to return the real context here rather than an opaque symbol as a token? It looks like you're going out of your way to make it opaque with types.

[dyladan]

Another way to impl may be to return a detach function which encapsulates the token. Keeps the implementation opaque and reduces the API to a single method addition, avoiding bugs caused by potentially calling detach with invalid input.