remove async storage from http2 server instrumentation #5947
Conversation
Overall package sizeSelf size: 9.61 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.8.2 | 9.56 MB | 9.93 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 | | import-in-the-middle | 1.14.0 | 120.58 kB | 841.68 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 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 | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.9 | 25.11 kB | 25.11 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 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-06-27 14:46:07 Comparing candidate commit 3a5a4b9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1048 metrics, 44 unstable metrics. |
Datadog ReportBranch report: ✅ 0 Failed, 1259 Passed, 0 Skipped, 13m 55.25s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5947 +/- ##
==========================================
- Coverage 79.60% 79.21% -0.40%
==========================================
Files 477 470 -7
Lines 20387 19983 -404
==========================================
- Hits 16229 15829 -400
+ Misses 4158 4154 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -16,7 +16,13 @@ class Http2ServerPlugin extends ServerPlugin { | |||
| this.addSub(`apm:${this.constructor.id}:server:${this.operation}:${eventName}`, handler) | |||
| } | |||
|
|
|||
| start ({ req, res }) { | |||
| addTraceBind (eventName, handler) { | |||
| this.addBind(`apm:${this.constructor.id}:server:${this.operation}:${eventName}`, handler) | |||
There was a problem hiding this comment.
Why is this needed in addition to the existing methods?
There was a problem hiding this comment.
bindStart and bindFinish never get called without adding this method.
There was a problem hiding this comment.
Why though? That should just work if everything is named properly. Maybe there is a typo somewhere?
There was a problem hiding this comment.
Yup, you're right, the naming was wrong, the tracingPlugin was using the channel prefix apm:http2:request, but the prefix should be apm:http2:server:request. I fixed it for http2 server and http server. And removed the addTraceSub method calls in the http plugins.
|
|
||
| return finishServerCh.runStores({ req: this.req, ...ctx }, () => { |
There was a problem hiding this comment.
The context should be altered and passed directly instead of creating a new object.
There was a problem hiding this comment.
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.
| @@ -92,7 +93,7 @@ const web = { | |||
| analyticsSampler.sample(span, config.measured, true) | |||
| }, | |||
|
|
|||
| startSpan (tracer, config, req, res, name) { | |||
| startSpan (tracer, config, req, res, name, traceCtx) { | |||
There was a problem hiding this comment.
I can't find where traceCtx is passed as a parameter in this PR.
| childOf = span | ||
|
|
||
| log.debug('Successfully created inferred proxy span.') | ||
|
|
||
| setInferredProxySpanTags(span, proxyContext) | ||
|
|
||
| if (traceCtx) { |
There was a problem hiding this comment.
This is repeated in a few places at this point. Could we maybe create a WebPlugin that would reuse the existing code instead in addition to call the util?
What does this PR do?
Motivation
Plugin Checklist
Additional Notes