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[bot]]

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). Report is 160 commits behind head on main.

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

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%)	:arrow_down:

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[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]

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

[mimiz]

Hello, is there a way to help for this PR ?

[DevinTyler26]

Hello, is there a way to help for this PR ?

+1, if there is something we could do to get this PR merged/release, let us know

[wesleytodd]

With v5 moving to latest, it would be great to land this for the folks asking. Hate to be another +1 comment, but it is unclear if there is something meaningful blocking this or if it is just limited maintainer time (which trust me, the answer is not more noisy comments I know). If there is a technical blocker, it might help to re-iterate that so any discussion can happen and this can be landed.

[timfish]

This can probably just ship as it is and we can migrate to a diagnostics_channel solution later.

Sentry have already shipped a version of this PR and users are already using it.

I'll fix up the conflicts and hopefully we can get this released.

[timfish]

Closing in favour of:

• https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2801

[trentm]

@timfish Thanks for doing this.

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.

Did you forgot to push changes to package.json that included showing running test/v5 tests?

In any case, I opened an alternative PR (https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2801) that builds on yours. Basically I took your hard work and cleaned up the testing story (and made the change to "src/instrumentation.ts" a little smaller). Now that the base supported Node.js for packages in this repo is Node.js 18, the testing story was a bit easier.

The original work is yours, so I'd be happy to updat this one to give due credit if you prefer.

(Though I see you are fast and closed this before I could post this comment. :)

[timfish]

Did you forgot to push changes to package.json that included showing running test/v5 tests?

Sorry I can't find anything!

The original work is yours, so I'd be happy to updat this one to give due credit if you prefer.

No problem, no credit required, I'm just happy someone has found some time to finish it!

[BorePlusPlus]

No problem, no credit required, I'm just happy someone has found some time to finish it!

You sir, are a gentleman and a scholar. :raised_hands: