ref(opentelemetry)!: Streamline OTEL context managment into setOpenTelemetryContextAsyncContextStrategy() - #22586
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I think hardening the _bindEventEmitter() checks would be good, and it's worth considering the ordering of precedence in the ALS argument passed in.
Also, maybe this is fine since it's just a stub and explicitly warns that it doesn't do anything, but I notice that the SentryAsyncLocalStorageContextManager has more functions now, and the stub at packages/opentelemetry/src/index.browser.ts doesn't implement them. Could be good to put some no-ops on it.
| (getAsyncContextStrategy(getMainCarrier()).getTracingChannelBinding?.() | ||
| ?.asyncLocalStorage as AsyncLocalStorage<Context>) ?? new AsyncLocalStorage<Context>(); | ||
| ?.asyncLocalStorage as AsyncLocalStorage<Context>) ?? | ||
| (asyncLocalStorage || new AsyncLocalStorage()); |
There was a problem hiding this comment.
If I'm reading this right, it'll only use the passed-in ALS if there isn't already an asyncLocalStorage on the ACS.
It's higher priority than creating a new one, but shouldn't it use the explicitly provided value as the highest priority?
3f7d8fa to
f20d4b0
Compare
f20d4b0 to
2474b3a
Compare
2474b3a to
9c0910d
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
2143fde to
f25d2c8
Compare
ca90926 to
c25e41a
Compare
|
@isaacs I updated this now to be more holistic, actually removing more stuff. for vercel-edge, turns out we do not need this globals stuff anymore and can just use async hooks directly, much nicer! |
mydea
changed the title
wrapContextManager exportwrapContextManager export
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some merge conflicts (likely due to oxlint changes, I think?) and a suggestion to harden the test a bit, but easily fixed and nothing worth gating on. Previous concerns addressed, looks good :)
| this._asyncLocalStorage = | ||
| (getAsyncContextStrategy(getMainCarrier()).getTracingChannelBinding?.() | ||
| ?.asyncLocalStorage as AsyncLocalStorage<Context>) ?? new AsyncLocalStorage<Context>(); | ||
| ?.asyncLocalStorage as AsyncLocalStorage<Context>) ?? new AsyncLocalStorage(); |
There was a problem hiding this comment.
This is a big improvement from last review, definitely solves the issue I raised.
The only remaining concern, which is much less, is that the ordering is load bearing. That is, if the strategy is set first, then the manager and the tracing-channel bindStore path will share a context, and everything's good. If not, then it silently falls back to a newly constructed instance, and the context is effectively hidden.
I checked the cases where this might happen, and it looks like they're all correct, so I don't think this is much of a hazard. But it might still be worthwhile to add a debug log here, if that does happen.
const existingALS: AsyncLocalStorage<Context> | undefined = getAsyncContextStrategy(getMainCarrier()).getTracingChannelBinding?.()?.asyncLocalStorage;
if (!existingALS) {
// debug log here
}
this._asyncLocalStorage = existingALS ?? new AsyncLocalStorage();There was a problem hiding this comment.
yeah, this is def. not ideal. the problem is that this is hard to make fully type safe I suppose as in browser this is not set but in other cases it should always be set I suppose 🤔 maybe we can harden this even more in a future iteration of this, making sure that this can never be set without having this defined on ACS 🤔 the debug log makes sense to me though, will add 🙏
There was a problem hiding this comment.
This is the reverse of how it works in production (see packages/vercel-edge/src/sdk.ts, where init sets up the ACS and then setupOtel does new SentryAsyncLocalStorageContextManager() later).
It's fine in the test, but would recommend swapping these two lines to match.
There was a problem hiding this comment.
Would probably also be good to add an assertion here to verify that manager.getAsyncLocalStorageLookup().asyncLocalStorage === getAsyncContextStrategy(getMainCarrier()).getTracingChannelBinding().asyncLocalStorage, since that's the goal, and would prevent it from regressing.
Instead, the context manager accepts the async local storage instance now, so we can also use this in vercel-edge. this will eventually go away/move to server-utils likely so we can revisit this in a follow up. actually other approach we just depend on async local storage rely on acs stuff for edge handling fixes fixes oops fix stuff small ref
mydea
changed the title
wrapContextManager exportsetOpenTelemetryContextAsyncContextStrategy()
c25e41a to
7c1fa7e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7c1fa7e. Configure here.
size-limit report 📦
|
|
I have refactored this further based on @isaacs feedback. Now, this removes some old cruft we had around for browser compatibility (which we no longer need/will have) and combines the acs + context manager code into a single call, which allows us to avoid some conditionals for implicit binding we had before which is now explicit. This leave some things in a bit of a weird in-between state - mostly, skipping otel setup will still set up the context manager now. but since this will fully go away this is fine right now IMHO. |
| }, | ||
| "dependencies": { | ||
| "@opentelemetry/api": "1.9.0", | ||
| "@opentelemetry/api": "1.9.1", |
There was a problem hiding this comment.
intermediate state: this is needed because the api versions need to align. will go away once this is no longer supported so this is OK for now.
2 participants
This removes browser compatibility from the
@sentry/opentelemetrypackage. The package is not usable in the browser anyway and will be fully removed as a browser target in the future — for now we drop the browser shim layer of the context strategy. Concretely, this removesindex.browser.ts(which only contained stubs that logged "not supported in the browser"), simplifies theexportsmap inpackage.jsonto plainimport/requireconditions, and drops the browser entrypoint from the rollup config.With the browser variant gone,
setOpenTelemetryContextAsyncContextStrategy()can own the whole context-management setup directly. It now creates theAsyncLocalStorageinstance, wires up theSentryAsyncLocalStorageContextManager, sets it as the global context manager, and returns the lookup — rather than having callers construct and register a context manager themselves and pass agetTracingChannelBindingfactory back in.Because the strategy now owns the
AsyncLocalStorage, we no longer need the separate node vs. custom async-context-strategy split (nodeAsyncContextStrategy.tsis removed), nor thewrapContextManagerClassshim used to graft Sentry scope handling onto an externally-provided context manager.For vercel-edge, it turns out we no longer need any custom handling for async local storage — since Next 13 this is allowlisted, so we can import it normally on the edge and just use the same default strategy as node. This lets us drop the vendored
abstract-async-hooks-context-manager/async-local-storage-context-managerfiles and theskipOpenTelemetrySetupspecial-casing in the edge SDK.The context manager also no longer checks for an
EventEmitterinstance (which does not work in vercel-edge) and instead inspects the shape of the object.Note that this uncovered a "bug" with the fs instrumentation, where it was instrumented too early. This was masked by undefined behavior due to
suppressTracing, as things were not fully set up yet.