feat(instrumentation-express): Add support for Express v5

[timfish]

Which problem is this PR solving?

Closes #2435

Short description of the changes

This PR adds support for express v5. The migration docs show few user facing API changes although the internals have changed quite a bit.

Express v5 has made Node v18 the minimum supported version although it looks like the tests only fail on v14 so far and >= v16 still pass.

The v5 tests are a copy of the v4 tests with some minor changes:

• Instrumented Express v5 emits middleware spans but the middleware - query and middleware - expressInit spans are missing, no doubt due to reworking of the internals.
• Some tests needed modifying because express no longer supports * paths. Instead /.*/ regex should be used.
• The complex regex tests expect the span names to contain '/test/arr/:id,/\\/test\\/arr[0-9]*\\/required(path)?(\\/optionalPath)?\\/(lastParam)?/' but a couple of them instead contain /test,6,/test/. This is either a bug in express or caused by a change to their regex support mentioned in the migration guide. In these instances, the integration does not have access to the "correct" path in any other property.

The v4 and v5 tests passed in isolation but when run though mocha together, many v4 tests failed with strange duplicate or missing spans. My best guess is that there's some incompatibility when running both express versions in the same process. To work around this I changed it to run these tests in isolated calls to mocha and then combine the nyc coverage in a posttest script.

[codecov]

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.80%. Comparing base (25e53d6) to head (4a71e9b).

Files with missing lines	Patch %	Lines
...try-instrumentation-express/src/instrumentation.ts	84.84%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2437      +/-   ##
==========================================
- Coverage   90.86%   90.80%   -0.07%     
==========================================
  Files         159      159              
  Lines        7849     7863      +14     
  Branches     1621     1627       +6     
==========================================
+ Hits         7132     7140       +8     
- Misses        717      723       +6     

Files with missing lines	Coverage Δ
...etry-instrumentation-express/src/internal-types.ts	100.00% <ø> (ø)
...opentelemetry-instrumentation-express/src/utils.ts	98.46% <100.00%> (ø)
...try-instrumentation-express/src/instrumentation.ts	94.83% <84.84%> (-3.75%)	⬇️

[joergbaier]

"@types/express": "4.17.21"

There is now a v5 for the typing too
https://www.npmjs.com/package/@types/express

[JamieDanielson]

Thanks for working on this! I have this on my (admittedly long) list of things to review. Right now my biggest concern is ensuring this doesn't disrupt folks using express 4.x, especially with some of the breaking changes in v5.

Express 5.x is still next (not latest) as they work through more migration guides and docs. For that reason I'm probably less concerned about the potential problems one may experience in the v5 instrumentation... just again I have an eye on v4 remaining relatively stable. If you have thoughts on how to further increase that confidence, or maybe examples of using it in a few larger projects on express v4, that could help!

[wesleytodd]

As I am not familiar with the structure of this project's internals, I am not sure my review adds much value, but here it is as requested in Slack (cc @AbhiPrasad ) even if it is small. If you are comfortable with the fact that your test might not be durable then I would say it all looks good. Now that we know the removed Route api is part of someones tests we can try to remember to notify you if we do change it, just can't promise anything.

[wesleytodd]

I am not sure I like this kind of check. I have been thinking about this problem as well because another package I maintain had compatibility for the v5 router but we broke that in the final push to release. I don't particularly like the idea of importing the package.json at startup in express, but we dont limit you from doing it. I am not familiar with your testing setup so I don't know how feasible that is for you to do. My main concern with this is we could decide to add a Route constructor back (not that I know of any plans to do so) which would break this.

[timfish]

Yeah agreed. I suspect it might be a better idea to target the two different versions separately 🤔

[wesleytodd]

Sorry should have fully read the PR description before posting a review.

The complex regex tests expect the span names to contain '/test/arr/:id,/\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/' but a couple of them instead contain /test,6,/test/. This is either a bug in express or caused by a change to their regex support mentioned in the migration guide. In these instances, the integration does not have access to the "correct" path in any other property.

Is that this? If so, where do I see the assertions for this? Sorry it is a long PR and I just skimmed mostly.

The new regular expression handling is here if you want to check. We moved it out of path-to-regexp entirely and dropped some things that relied on partial matching behaviors (which were part of the redos issues). Let me know if you find a bug in there, but it might be we consider it intentional depending on how you are using it (which I did not see in an obvious way in this PR).

[timfish]

Is that this? If so, where do I see the assertions for this?

Here are the assertions which have been changed to pass.

And here is the regex request handler that these are tested against.

For v4, these paths resulted in COMMON_PATH. You can see the last two now result in GET '/test,6,/test/':

['arr/requiredPath/optionalPath/', '/test,6,/test/'],
['arr/requiredPath/optionalPath/lastParam', '/test,6,/test/'],

[timfish]

I've improved the code to instrument differently depending on the express version but like for v4, the v5 instrumentation still relies on internals.

One option would be to emit tracing events directly from express via diagnostics_channel now that Node version support would allow this. This would mean no more relying on internals!

[wesleytodd]

One option would be to emit tracing events directly from express via diagnostics_channel now that Node version support would allow this. This would mean no more relying on internals!

This is the goal!! We will be putting together a group of folks interested in this and working on it in the express project. For anyone interested, there is an onging slack conversation you can join in the OpenJS slack: https://openjs-foundation.slack.com/archives/C02QB1731FH/p1729517144349069