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.
|
7b409c3 to
8d14096
Compare
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>
c12663f to
705e3c1
Compare
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.