-
Notifications
You must be signed in to change notification settings - Fork 344
Removing hook for express v5 router #6272
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: 11.92 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.1.0 | 20.37 MB | 20.37 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.9.0 | 9.77 MB | 10.1 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.4 | 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 | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6272 +/- ##
==========================================
- Coverage 83.99% 78.77% -5.22%
==========================================
Files 477 472 -5
Lines 20071 20027 -44
==========================================
- Hits 16858 15776 -1082
- Misses 3213 4251 +1038 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
540368b
to
e0a5c37
Compare
BenchmarksBenchmark execution time: 2025-08-29 16:39:46 Comparing candidate commit acb9b94 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 1264 metrics, 58 unstable metrics. scenario:appsec-iast-no-vulnerability-control-22
|
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 see what you're trying to do here, but I believe there's an issue with this change.
By removing the addHook
blocks that scoped the wrapping logic to specific Express versions, we're now applying the same wrapping logic to express.Router
regardless of whether it's Express 4 or 5. This isn't safe: in Express 5, Router is a constructor and the wrapping should happen on its prototype, while in Express 4 it's not, so wrapping it directly is necessary.
Applying the same wrap to both versions lead to runtime errors, as seen in the tests.
If the goal is to avoid wrapping express.Router
entirely in Express 5, we should retain the version-specific addHook
blocks and simply omit the wrapping for Router in the >=5.0.0
block.
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.
@CarlesDD I believe I addressed your issue, I have added in an addHook
block for express v4 with a comment as well. This is will make it more explicit that for v4 we are wrapping the router and in v5 we will fall to the router instrumentation to wrap the calls.
18c3669
to
7eec462
Compare
7eec462
to
acb9b94
Compare
What does this PR do?
This PR will remove the hook for Express v5 router. In v5 the router was separated out into its own module and right now we are wrapping it as well as wrapping the router module. This is resulting in double instrumentation for router and thus double spans. This will remove the instrumentation of the router in express and it will fall to just instrumenting the router.
Motivation
There is a current escalation where the customer had double spans.
Plugin Checklist
Additional Notes