-
Notifications
You must be signed in to change notification settings - Fork 539
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
fix(instr-express): fix handler patching for already patched router #2294
fix(instr-express): fix handler patching for already patched router #2294
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2294 +/- ##
==========================================
- Coverage 90.97% 90.42% -0.56%
==========================================
Files 146 148 +2
Lines 7492 7322 -170
Branches 1502 1518 +16
==========================================
- Hits 6816 6621 -195
- Misses 676 701 +25
|
plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
…entation.ts Co-authored-by: Abhijeet Prasad <[email protected]>
Thank you! I've done the patch locally and tested with Ghost, things are working swimmingly. |
Fixed the lint issue. Ready for review @JamieDanielson @pkanal |
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.
Thanks for this @david-luna ! Also appreciate the new test and detail in the comments.
Which problem is this PR solving?
Short description of the changes
handler
(own properties and inherited)I finally opted to use the
for...in
statement instead of theProxy
. I ran some benchmarks following this article and the results in node v18.16.0 wereSince any middleware can access the
handler
properties I think there could be use cases where those properties are accessed frequently so I preferred thefor...in + defineProperty
approach. The only drawback is non enumerable props are not accessible but I think for now it's fine