Skip to content
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

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

I finally opted to use the for...in statement instead of the Proxy. I ran some benchmarks following this article and the results in node v18.16.0 were

% node index.js

vanilla x 1,107,664,482 ops/sec ±0.21% (101 runs sampled)
proxy x 3,842,562 ops/sec ±0.34% (75 runs sampled)
defineProperty x 1,108,137,480 ops/sec ±0.25% (101 runs sampled)
Fastest is defineProperty,vanilla

Since any middleware can access the handler properties I think there could be use cases where those properties are accessed frequently so I preferred the for...in + defineProperty approach. The only drawback is non enumerable props are not accessible but I think for now it's fine

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (dfb2dff) to head (db19a98).
Report is 184 commits behind head on main.

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     
Files Coverage Δ
...try-instrumentation-express/src/instrumentation.ts 98.61% <100.00%> (-1.39%) ⬇️

... and 61 files with indirect coverage changes

@Zirak
Copy link
Contributor

Zirak commented Jun 25, 2024

Thank you! I've done the patch locally and tested with Ghost, things are working swimmingly.

@david-luna
Copy link
Contributor Author

Fixed the lint issue. Ready for review @JamieDanielson @pkanal

Copy link
Member

@JamieDanielson JamieDanielson left a 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.

@JamieDanielson JamieDanielson enabled auto-merge (squash) July 9, 2024 23:06
@JamieDanielson JamieDanielson merged commit 2c32e58 into open-telemetry:main Jul 9, 2024
1 check passed
@dyladan dyladan mentioned this pull request Jul 9, 2024
@david-luna david-luna deleted the dluna/2271-fix-express-handler-prototype branch July 10, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Express instrumentation results in routes with no stack, crashing Ghost
5 participants