-
Notifications
You must be signed in to change notification settings - Fork 340
remove async storage from http2 server instrumentation #5947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 9.69 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.0.1 | 20.3 MB | 20.3 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.9.0 | 9.77 MB | 10.14 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.2 | 122.36 kB | 850.93 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2025-07-23 16:53:48 Comparing candidate commit 2d4d910 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1277 metrics, 46 unstable metrics. |
Datadog ReportBranch report: ✅ 0 Failed, 1259 Passed, 0 Skipped, 13m 55.25s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5947 +/- ##
==========================================
- Coverage 81.98% 80.45% -1.53%
==========================================
Files 475 259 -216
Lines 19598 9733 -9865
==========================================
- Hits 16067 7831 -8236
+ Misses 3531 1902 -1629 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
return finishServerCh.runStores({ req: this.req, ...ctx }, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context should be altered and passed directly instead of creating a new object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if I understand the new code correctly, it now restores the parent context here where before it was restoring the current context. This means event listeners will no longer be in the context of the span. I'm not sure which is the expected behaviour, especially if there was no test for this, but to be safe it's probably best to keep the previous behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this behavior, now it restores the current context.
@wconti27 I believe the system test failures are related to the current implementation. If that's the case, I guess we are missing local tests, since it would be good to be aware about the behavior in our direct tests as well. As such, it would be great if additional tests are added as part of this PR when the failures are fixed. |
Its odd, we do have tests. But they said they passed even though I had originally messed up the start span call and it was failing to run and create inferred proxy spans. Looking into it now They were run here: https://github.com/DataDog/dd-trace-js/actions/runs/16197859525/job/45728961958?pr=5947#:~:text=packages/dd%2Dtrace,to%20be%20off |
@BridgeAR Fixed this, we previously were catching the thrown error in the callback of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change overall LGTM, just a couple of small things to address and it's good to go!
@@ -102,25 +102,28 @@ class TracingPlugin extends Plugin { | |||
} | |||
} | |||
|
|||
startSpan (name, { childOf, kind, meta, metrics, service, resource, type } = {}, enterOrCtx = true) { | |||
startSpan (name, options = {}, enterOrCtx = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startSpan (name, options = {}, enterOrCtx = true) { | |
startSpan (name, options = {}, enterOrCtx = true) { | |
let { childOf, kind, meta, metrics, service, resource, type } = options |
It's best to not alter object received as a parameter to a function. This also means restoring direct usage below.
const TracingPlugin = require('../tracing') | ||
|
||
function startSpanHelper (tracer, name, options, traceCtx, config = {}) { | ||
return TracingPlugin.prototype.startSpan.call( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an anti-pattern, prototype methods are meant to become available on instances of the class, not be accessed directly.
What does this PR do?
Motivation
Plugin Checklist
Additional Notes