feat(context): add attach and detach methods to context API

[dyladan]

This adds attach/detach methods to context API as they are specified in optional global operations.

Non-breaking for users as this is an additive change.

Breaking for SDK implementers. SDKs must implement the attach/detach methods.

[codecov[bot]]

Codecov Report

Base: 95.23% // Head: 94.82% // Decreases project coverage by -0.40% :warning:

Coverage data is based on head (e6dbc6d) compared to base (d2fb706). Patch coverage: 57.14% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
- Coverage   95.23%   94.82%   -0.41%     
==========================================
  Files          45       45              
  Lines         650      657       +7     
  Branches      102      102              
==========================================
+ Hits          619      623       +4     
- Misses         31       34       +3     

Impacted Files	Coverage Δ
src/api/context.ts	90.32% <50.00%> (-5.98%)	:arrow_down:
src/context/NoopContextManager.ts	94.44% <66.66%> (-5.56%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[Flarna]

Have you verified that we can implement these APIs with the existing context managers (AsyncHooks, zone.js)? If not this would be quite problematic.

[vmarchaud]

Have you verified that we can implement these APIs with the existing context managers (AsyncHooks, zone.js)? If not this would be quite problematic.

Looks doable to me for async hooks at least

[Flarna]

I think we should work on prototypes for async hooks and zone.js before adding this to API. Otherwise the risk is too high to loose support for a major part.

[dyladan]

Have you verified that we can implement these APIs with the existing context managers (AsyncHooks, zone.js)? If not this would be quite problematic.

Yes I have implemented prototypes in the async hooks and async local storage context managers. It was actually much simpler than I expected. I have not tried in the zone context manager yet.

I think we should work on prototypes for async hooks and zone.js before adding this to API. Otherwise the risk is too high to loose support for a major part.

I also think that's a good idea, but I'm not sure how to expose it for use without adding it to the API. If it is never used, then we will never learn anything from it. Maybe we should add a prefix to the API methods like api.context.experimental.attach?

[Flarna]

We could move API into core repo and link them via lerna :o).

I think the development of such an API must start go hand in hand with core anyway otherwise core breaks automatically as it uses "@opentelemetry/api": "^1.0.2" as dependency. Adding a new mandatory API would break CI once API is published (could be avoided by pinning API dependency in core).

I see two variants:

• merge API and publish a canary version to NPM and use it in core. Once core is done do an API release.
• create API proposal here. Once it settled implement in core and merge to master (maybe even publish to NPM is needed). Once this is done proceed with API.

Yes I have implemented prototypes in the async hooks and async local storage context managers

Is the semantic of asyncLocalStorage.exit and asyncLocalStorage.enterWith the same as that one specified here (e.g. in case of nested calls, "wrong" enter/exit sequence,...)?

I have not tried in the zone context manager yet.

I did a fast look into the API but found nothing like enterWith/exit but I'm not really familiar with zone.js.

[Flarna]

Breaking for SDK implementers. SDKs must implement the attach/detach methods.

Created https://github.com/open-telemetry/opentelemetry-js/pull/2531 to avoid automatic breaking of the OTel SDK.

[blumamir]

To summarize my biggest concern with the current direction is that, to my understanding, the attached key will only be active for the duration of the top most wrapping context.with call. When the context.with callback terminate, the previous context will be restored and will not contain the attached key. That might come as a surprise to users and be counterintuitive.

I think the expectation of the user is that the context remains attached for any code that "follows" the call. For example - if it was set in a framework's (like fastify) middleware it should also be active on any downstream operation like other middlewares and request handlers. If the key is lost when the middleware returns and the middleware span's context is ended, then it's not solving the need IMO.

[dyladan]

I think not calling detach after calling attach should be considered a bug. You can't expect your context that you attach'd to survive the end of a with.

You gave the following example:

const contextKey = Symbol('example-context-key');

const g = async () => {
    context.attach(context.active().setValue(contextKey, 'example context value'));
}

const f = async () => {
    await g();
    console.log('since g is async, the context attached in g will not be active here');
}

I'm not actually sure this is correct. I'll test it with my prototype implementations in the context managers.

[Flarna]

A detach happens automatically if the current JS tick function ends. So it might be correct to not call detach if you want the context active until the resource vanishes.

There are usecaes when your code runs in a callback of some framework and you want that everything - including the parts done by the framework after your handler returns - are linked to your context. In a lot cases you can archive this by patching something deep in the framework which covers all these parts. e.g the HTTP instrumentation wraps the request handler by patching createServer. A less intrusive/patchless variant would be to just prepend a request listener and do an attach there.

But this comes with high risk. The framework in use may do quite some work after the user part returned. And the user who didn't a detach should know what happens there and be sure that the context should be really active in these code parts.

[weyert]

@open-telemetry/javascript-maintainers How can we progress this PR? Looks useful

[dyladan]

@open-telemetry/javascript-maintainers How can we progress this PR? Looks useful

The issue is that a lot of the places where this PR looks useful it is actually not. It may be useful in some situations but I don't know if it actually helps. One big issue mentioned earlier by @Flarna is that if you are in a function which was called with context.with and you use attach, at the end of the function the original context from before the function was called will be restored and your context will be lost. This may create confusion for users who don't know why their context is being lost. It could also introduce the possibility for very nasty context bugs if attach and detach are called with wrong ordering.

[stefee]

I think this might be a hard requirement for https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1021 - until then I don't know if there is a workaround for Undici users other than to manually add propagation headers on outgoing requests.

[vmarchaud]

But I suppose that AsyncLocalStorageContextManager already utilized experimental APIs like AsyncLocalStorageContextManager.disable [1] so it can be acceptable?

I mean we started using ALC when it was still experiemental so i don't think its an issue by itself. I would even add that attach/detatch would not be used most of time so even if there are an issue with the underlying implementation, it would not be a problem for the majority of our users ?

[dyladan]

But I suppose that AsyncLocalStorageContextManager already utilized experimental APIs like AsyncLocalStorageContextManager.disable [1] so it can be acceptable?

I mean we started using ALC when it was still experiemental so i don't think its an issue by itself. I would even add that attach/detatch would not be used most of time so even if there are an issue with the underlying implementation, it would not be a problem for the majority of our users ?

We can also add a disclaimer to the JS docs that it uses experimental APIs

[dyladan]

Added experimental tags to the docs and verified the rendered docs look as expected. If the other maintainers don't think this is a strong enough signal to end-users, we can consider making this method private which would require use of the ["property"] syntax for typescript users.

[vmarchaud]

Added experimental tags to the docs and verified the rendered docs look as expected

I think that's fine, i mean in the end people that want to use it will do it even if we had some protections so lets just mark it as experimental and move forward, even if someone complain we would just redirect to the fact that it was experimental. Apart from https://github.com/open-telemetry/opentelemetry-js-api/pull/123#discussion_r732716054, lgtm

[dyladan]

@open-telemetry/javascript-maintainers what do you think of the proposal here: https://github.com/open-telemetry/opentelemetry-js-api/pull/123#discussion_r915988746

Use would look like this:

const restore = context.attach(newContext);
// do some stuff
restore(); // detach context

[arminru]

@dyladan Can this one be closed as per https://github.com/open-telemetry/community/issues/1290?