opentelemetry-js-contrib icon indicating copy to clipboard operation
opentelemetry-js-contrib copied to clipboard

fix(instr-express): keep hidden properties in layer handlers

Open david-luna opened this issue 1 year ago • 2 comments

Which problem is this PR solving?

  • The original handle function of an express layer may contain certain metadata, specially in the router layer. When instrumentation does the "patch" the patched function does not have the same properties so any code relying on those props will find nothing. That may not be the case in express itself but 3rd party tools that look into that metadata are failing.

Fixes: #1950

Short description of the changes

  • The patched function now exposes same properties proxying the ones from the original handle function.
  • A test has been added to check the stack property is available for the router layer.

david-luna avatar Apr 22 '24 17:04 david-luna

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.37%. Comparing base (dfb2dff) to head (49fd215). Report is 155 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2137      +/-   ##
==========================================
- Coverage   90.97%   90.37%   -0.61%     
==========================================
  Files         146      149       +3     
  Lines        7492     7529      +37     
  Branches     1502     1576      +74     
==========================================
- Hits         6816     6804      -12     
- Misses        676      725      +49     
Files Coverage Δ
...etry-instrumentation-express/src/internal-types.ts 100.00% <ø> (ø)
...try-instrumentation-express/src/instrumentation.ts 98.61% <80.00%> (-1.39%) :arrow_down:

... and 52 files with indirect coverage changes

codecov[bot] avatar Apr 22 '24 17:04 codecov[bot]

... will decrease coverage by 0.65%.

This is due to no tests are manipulating the hidden properties of the handle function of the layers. so the setter is not covered in the tests.

Screenshot 2024-04-23 at 10 18 27

I can remove the setter since until now not having the property at all was not breaking apps. But I think is a matter of time to get a new issue because of a lib trying to update those rops

david-luna avatar Apr 23 '24 08:04 david-luna

Note to self: this issue will probably get fixed by this PR. Check it

david-luna avatar Jun 05 '24 11:06 david-luna