feat(instrumentation-express): add support for Express v5, take 2

[trentm]

This PR starts with @timfish's #2437 (Tim did all the hard work) and
makes it a smaller patch -- mostly by avoiding having separate
test/v4 and test/v5 trees.

Now that we are on JS SDK v2 and hence only support Node.js v18 as
a minimum (the same as express@5) the testing story is now simpler:
no need to avoid running some tests with older Node.js versions.
(See #2722 for a general discussion of the pain when having to do that.)

Obsoletes: #2437
Closes: #2435

Testing Notes

• I've bumped the express version in package.json to v5, so that version is tested via npm test.
• I've updated .tav.yml so that supported express@4 versions are tested via npm run test-all-versions.

(If there is a coverage failure due to this, I'll make the argument that we can ignore it. Given our instrumentations have version-specific code, the only reasonable way to get full coverage is to gather coverage from npm run test-all-versions runs and not from npm test runs.)

[trentm]

Some dev notes.

[trentm]

Dev Note:

Interesting side-bar. The following route in "test/fixtures/use-express-regex.mjs" (added in #2008) was testing whether a request path /test/arr/requiredPath/optionalPath/ matched that route. Notice the capital letter in requiredPath. Normally this should NOT match. However it does in express@4, not in express@5 and I think it is an express@4 limitation/bug.

app.get(
  [
    '/test/arr/:id',
    /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/,
  ],
  (_req, res) => {
    res.send({ response: 'response' });
  }
);

Let's use a simpler example (note to self: /Users/trentm/tmp/asdf.20250424T102536/app.js):

app.get(
  [
    '/foo',
    /^\/another-path$/,
  ],
  (req, res) => {
    res.end('pong');
  }
);

• Express routes are case-insensitive by default .
• express@4 used [email protected] for its route matching. That version of the lib creates a single regexp to match for any of the entries in the route array. So how to match '/foo' case-insensitively, but '/another-path' case-sensitively? The answer is that you don't. This is the regexp used in express@4 for matching that route: /^\/foo\/?$|^\/another-path$/i. It is all case-insensitive. In other words, by including a regexp route together with a string route in an array to app.get(...) results in the regexp route being made case-insensitive.
• In express@5 the router@2 module is used for route matching and it handles each of the '/foo' and /^\/another-path$/ entries separately.

The existing test data here was unintentionally relying on that Express routing subtlety. That subtlety changed in express@5, but isn't relevant to the testing here.

[JamieDanielson]

Thanks for clarifying this!

[codecov]

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.46%. Comparing base (64d358e) to head (d83a070).
Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2801      +/-   ##
==========================================
- Coverage   89.53%   89.46%   -0.07%     
==========================================
  Files         180      180              
  Lines        8725     8735      +10     
  Branches     1771     1778       +7     
==========================================
+ Hits         7812     7815       +3     
- Misses        913      920       +7     

Files with missing lines	Coverage Δ
...opentelemetry-instrumentation-express/src/utils.ts	97.10% <100.00%> (ø)
...try-instrumentation-express/src/instrumentation.ts	96.75% <80.00%> (-1.86%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[trentm]

^^ toggle the pkg:instrumentation-express label to get the TAV tests running.

[pichlermarc]

cc @raphael-theriault-swi @JamieDanielson @pkanal (component owners)

[pichlermarc]

This will likely also fix open-telemetry/opentelemetry.io#6726

[wesleytodd]

I don't have much to include just like on the last one, but if there is some better way we can export the express version from the library I am down for landing that to help prevent you from having checks like that one.

[wesleytodd]

4.16.2 is not supported anymore, ideally bump this to 4.21.2.

[JamieDanielson]

True, although if 4.16.2 still works without issue I'm okay keeping this until it causes us problems, unless it is massively increasing testing time.

[wesleytodd]

I just mentioned it because of these: https://github.com/expressjs/express/security/advisories?state=published

[wesleytodd]

I guess I should have posted more context. I doubt any impact from CVEs here. I just like to point it out in case there are parts of the systems I am unaware of that if it is not done for a real reason it is best to just avoid the versions with published CVEs. Hope that helps clarify my reasoning for mentioning it.

[JamieDanielson]

Ah good callout! Thanks for pointing that out. I don't think we have a strict process we follow on when to drop package versions from contrib instrumentations. It's unfortunate that we don't have a way to track how many folks are on that version, though we're not technically removing the ability to use it, just narrowing the test scope. Now is probably a good time to remove it from testing.

[wesleytodd]

I don't think we have a strict process we follow on when to drop package versions from contrib instrumentations.

We have worked on an LTS plan which includes things like when partners should drop support for integrations. Maybe this helps? https://github.com/expressjs/discussions/blob/master/docs/adr/lts-strategy.md

In this case, I think this is the relevant line:

Users are required to follow the head/latest of each major release line for support with all packages (express, dependencies, middleware, & tools/other)

It's unfortunate that we don't have a way to track how many folks are on that version

I don't know what scope you mean here, but the downloads tab on npm gives a rather broad view of that: https://www.npmjs.com/package/express?activeTab=versions

though we're not technically removing the ability to use it, just narrowing the test scope. Now is probably a good time to remove it from testing.

All in all I think this is a good choice if it is just impacting testing. If a user comes in broken and says "well I haven't updated express in 5 years" I think they will be better off going to upgrade. Win win!

[JamieDanielson]

downloads tab on npm

Ah yep, I meant specifically folks who use this instrumentation. But to your point, this version is overall pretty low in terms of downloads compared to even the next minor release. Besides, if someone is still on this old of a version of express, they are less likely to be upgrading this dependency. Let's increase minimum testing version 👍

[trentm]

I disagree slightly and I'd like to defer changing this to a separate PR so this one can stay on "add support for express@5".

#2809

[JamieDanielson]

Thank you @trentm and @timfish for all your hard work on this! I've added to the other comments regarding minimum testing version and feature-sniffing vs semver check, but they are nonblocking. Express v5 is now latest, let's make sure we can instrument it 🚀

[Ecksters]

README is still indicating:

Supported Versions
express version >=4.0.0 <5

And I seem to be having trouble getting it to work with Express 5.1, not sure if the README is correct or if I'm doing something wrong on my end.

[trentm]

@Ecksters I only fluked upon your comment here. I've opened #3159 to update the README.md.

And I seem to be having trouble getting it to work with Express 5.1,

If you are still having an issue, please open a new issue with details so we can take a look. My understanding is things should be working with [email protected]. For example this recentish workflow run includes a test of instr-express with [email protected]: https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/18317653219/job/52162813315