feat(opentelemetry): Vendor AsyncLocalStorageContextManager#20243
feat(opentelemetry): Vendor AsyncLocalStorageContextManager#20243
AsyncLocalStorageContextManager#20243Conversation
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.
|
mydea
force-pushed
the
fn/vendor-otel-contextmanager
branch
from
7b409c3 to
8d14096
Compare
isaacs
left a comment
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?
This vendors in the required code from `@opentelemetry/context-async-hooks`
Co-authored-by: isaacs <i@izs.me>
mydea
force-pushed
the
fn/vendor-otel-contextmanager
branch
from
c12663f to
705e3c1
Compare
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>
|
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. |
|
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 |
4 participants
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.