Uh oh!
There was an error while loading. Please reload this page.
feat(opentelemetry): Vendor AsyncLocalStorageContextManager - #20243
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛Deno
Other
Internal Changes 🔧Ci
Deps
Other
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
7b409c3 to
8d14096Compare
isaacs
left a comment
There was a problem hiding this comment.
This looks good to me, and combining two external deps into one internal is very nice.
| configurable: true, | ||
| writable: false, | ||
| value: target.length, | ||
| }); |
There was a problem hiding this comment.
I am guessing that this is here to handle the express/connect pattern of treating (er, req, res, next) => {} differently from (req, res, next) => {}, but is it worth considering patching toString() as well, since we do that with our wrapped functions?
There was a problem hiding this comment.
hmm I basically just copy-pasted this from the original implementation - can you elaborate what exactly you mean? Can't quite find where we do this today?
Uh oh!
There was an error while loading. Please reload this page.
This vendors in the required code from `@opentelemetry/context-async-hooks`
Co-authored-by: isaacs <i@izs.me>
c12663f to
705e3c1CompareUh oh!
There was an error while loading. Please reload this page.
This vendors in the required code from `@opentelemetry/context-async-hooks`. This also deprecates the `wrapContextManagerClass` method. It will continue to work until v11, but will be removed then - you'll no longer be able to use that, instead you'll have to use the provided context manager for integrated setup. Old code should continue to work as expected, but `@sentry/node` has one dependency less (and node-core one peer dependency less) and should work the same as before. --------- Co-authored-by: isaacs <i@izs.me>
rwe
commented
Apr 22, 2026
This now makes it impossible (or requiring build hacks at least) to use otel in a browser build, whereas before it could, even using the The single import{AsyncLocalStorage}from'node:async_hooks';import{EventEmitter}from'node:events';whereas before it only imported compatible |
Hey, you can still use the same procedure for this as before, the Or are you saying that this fails at build-time for you? 🤔 if this is the case, then yes we should fix this - please open an issue here with as much details as possible about your build system, then we'll take a look, if that is what's happening 🙏 |
Hi @mydea thanks for the response! Yes, it fails at build time since those I was able to work around with some webpack surgery+stubs since they mostly aren't needed at runtime. But I wonder whether this means that browser isn't even intended to be a supported runtime environment, in which case I'll need to reconsider our approach. Will open an issue when back to keyboard. |
mydea
commented
Apr 28, 2026
I opened a PR here which should fix this - as long as your bundler/setup is correctly interpreting conditional exports (which all that we support should) then the node-specific things should no longer be pulled in for browser envs! #20556 |
This vendors in the required code from
@opentelemetry/context-async-hooks.This also deprecates the
wrapContextManagerClassmethod. It will continue to work until v11, but will be removed then - you'll no longer be able to use that, instead you'll have to use the provided context manager for integrated setup.Old code should continue to work as expected, but
@sentry/nodehas one dependency less (and node-core one peer dependency less) and should work the same as before.