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

[david-luna]

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]

Note for reviewer: this usage of Function.protptype.call comes from this commit 0723d06 as a new addition.

I did not dig further to get the original implementation but I think there is no need to use .call(...) since we have access to the container object and therefore calling _applyPatch with the right context.

[codecov]

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%)	⬇️

... and 52 files with indirect coverage changes

[david-luna]

... 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.

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

[JamieDanielson]

Thanks @david-luna ! I will review this week. In the meantime can you run npm run lint:fix to update the lint errors?

[JamieDanielson]

While I understand the purpose of what we're trying to achieve, the error that comes up here feels very relevant and makes me wonder if there's a better way to do this.

[david-luna]

@JamieDanielson

Thanks for your review :)

...makes me wonder if there's a better way to do this.

For "better" you meant at the type level or you think the usage of Object.defineProperty is not a good solution? I'll put my thoughts on both.

Type error
The type error we get is:

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Function'.
  No index signature with a parameter of type 'string' was found on type 'Function'.ts(7053)

The internal typings define the handle property of the layer as a Function. I've also checked @types/express to see if we can get the type from there but the router stack (where the layers are stored) is defines as any[]. So at the time of writing it adding comment to highlight express is "leveraging" the Object nature of functions to hide metadata felt the right thing to do. Note: the type error also arises with @blumamir's proposal since its a type level error.

We may get rid of the error by doing some improvements in the internal types so the compiler will know the function being patched is also used as a record. Something like the type below should work.

type HandlerType = Function & Record <string,any>

Object.defineProperty vs copy

Doing the shallow copy proposed by @blumamir fixes this issue as well but those properties represents the state of the layer. If that state is modified for the handler to change it's behaviour the original one will not notice that change and keep behaving according the original state (before the copy). With defineProperty we keep patched and original in sync (sharing state) so the instrumentation will not interfere with any code accessing or changing the state.

[JamieDanielson]

Ooh, that sounds like a good idea to use that new HandlerType. Would you be open to adding that into internal-types, and using that instead of ignoring the TypeScript error?

Also thank you for the detailed explanation, that helps me understand much better!

[blumamir]

Ooh, that sounds like a good idea to use that new HandlerType. Would you be open to adding that into internal-types, and using that instead of ignoring the TypeScript error?

Also thank you for the detailed explanation, that helps me understand much better!

I support doing this

[david-luna]

I'll update the types :)

[david-luna]

Done @JamieDanielson :)

[david-luna]

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