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

trentm · 2025-04-25T19:05:31Z

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

@timfish

This PR starts with @timfish's open-telemetry#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 open-telemetry#2722 for a general discussion of the pain when having to do that.) Obsoletes: open-telemetry#2437 Closes: open-telemetry#2435

trentm

Some dev notes.

trentm · 2025-04-25T19:07:20Z

plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

+          const isExpressV5 =
+            typeof moduleExports?.Router?.prototype?.route === 'function';


Dev note: I used feature-sniffing to detect the version. We could use (moduleExports, version) => { ... and a semver.satisfies() check of the loaded module version if there is a preference for that.

This is very brittle, but I don't know enough about your InstrumentationNodeModuleDefinition wrapper to be sure. Is this simply a check to tell which version it is? Or do you have logic somewhere based on if .route is a function?

I guess the main question here is whether that feature may be backported at any point, removing the reliability of this check. Considering v4.x has been out for... years... I wouldn't rule out backporting features as a possibility. I'd lean toward version if we can, similar to how we do it in MongoDB.

Alternatively if we can comment the reasoning with a link to docs somewhere that might be good enough and we can change to use version if it causes a problem later with a backport.

Also just realized you used the semver.satisfies() check in testing. I like the explicitness of it anyway.

I guess the main question here is whether that feature may be backported at any point

No it will not be backborted, but we want to clean up some of these interfaces in the future and so understanding how users are using these api's (especially incorrectly like this for version checking) is important for us to keep in mind.

Sorry, I am not sure if I was clear before, but I am one of the TC members for express, that's why I was pinged on the other PR and so got a notification for this one. Feel free to ask me if you have questions for the express side.

Is this simply a check to tell which version it is? Or do you have logic somewhere based on if .route is a function?

The instrumentation here wraps methods on the Router prototype, including .route (just below this const isExpressV5 = ... statement), so it feels reasonable to (somehow) check that the thing to be instrumented exists.

If express@5 internals changed such that Router.prototype.route went away, then this instrumentation-express implementation would have to change. So, IIUC, a semver check something like semver.satisfies(version, '^5') wouldn't really help for future-proofness either.

@wesleytodd Or am I missing a detail in which you think this is more brittle than the semver-based differentiation that the previous PR was doing: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2437/files#diff-81c3e13c3dac53e0dd6aabf6dc165305350eb522a85a2561f9ee51d9249011e7R84-R90

If the var was called isExpressWithRouterPrototype or something like that, would that be clearer?

The instrumentation here wraps methods on the Router prototype, including .route (just below this const isExpressV5 = ... statement), so it feels reasonable to (somehow) check that the thing to be instrumented exists.

Ah ok yes, with this explanation I agree it makes sense. Calling it isExpressV5 is what threw me as I thought maybe it could be used for other things related to other parts of the v5 api changes (spoilers for below...).

If express@5 internals changed such that Router.prototype.route went away,

We cannot do that as it would be breaking. So you are for sure safe on that until the next major.

So, IIUC, a semver check

And I agree I am not sure a semver check would be better since you use this for is to patch .route.

If the var was called isExpressWithRouterPrototype or something like that, would that be clearer?

Yes, after reading your first line that is where my head went before I even read down to the end of your reply. 🎉

trentm · 2025-04-25T19:12:08Z

plugins/node/opentelemetry-instrumentation-express/test/express.test.ts

-    'arr/requiredPath/optionalPath/',
-    'arr/requiredPath/optionalPath/lastParam',
+    'arr/requiredpath/optionalPath/',
+    'arr/requiredpath/optionalPath/lastParam',


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 path-to-regexp@0.1 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.

Thanks for clarifying this!

codecov · 2025-04-25T19:14:20Z

Codecov Report

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

Project coverage is 89.53%. Comparing base (32f41ee) to head (343b949).
Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2801      +/-   ##
==========================================
+ Coverage   89.50%   89.53%   +0.03%     
==========================================
  Files         180      180              
  Lines        8719     8727       +8     
  Branches     1767     1773       +6     
==========================================
+ Hits         7804     7814      +10     
+ Misses        915      913       -2

Files with missing lines	Coverage Δ
...opentelemetry-instrumentation-express/src/utils.ts	`97.10% <100.00%> (ø)`
...try-instrumentation-express/src/instrumentation.ts	`97.36% <84.61%> (-1.25%)`	⬇️

... 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 · 2025-04-25T19:16:28Z

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

pichlermarc · 2025-04-30T08:58:27Z

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

pichlermarc · 2025-04-30T09:08:09Z

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 · 2025-04-30T13:33:16Z

plugins/node/opentelemetry-instrumentation-express/.tav.yml

@@ -1,5 +1,5 @@
 express:
  - versions:
-      include: "^4.16.2"
+      include: ">=4.16.2 <6"


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

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.

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

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.

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.

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!

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 👍

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

wesleytodd · 2025-04-30T13:38:26Z

plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

+          const isExpressV5 =
+            typeof moduleExports?.Router?.prototype?.route === 'function';


This is very brittle, but I don't know enough about your InstrumentationNodeModuleDefinition wrapper to be sure. Is this simply a check to tell which version it is? Or do you have logic somewhere based on if .route is a function?

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 🚀

trentm self-assigned this Apr 25, 2025

trentm requested a review from a team as a code owner April 25, 2025 19:05

github-actions bot assigned JamieDanielson, pkanal and raphael-theriault-swi Apr 25, 2025

github-actions bot added the pkg:instrumentation-express label Apr 25, 2025

github-actions bot requested review from JamieDanielson, pkanal and raphael-theriault-swi April 25, 2025 19:05

timfish mentioned this pull request Apr 25, 2025

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

Closed

trentm commented Apr 25, 2025

View reviewed changes

trentm added pkg:instrumentation-express and removed pkg:instrumentation-express labels Apr 25, 2025

david-luna approved these changes Apr 29, 2025

View reviewed changes

pichlermarc mentioned this pull request Apr 30, 2025

[Page feedback]: Install command installs incompatible versions of the app open-telemetry/opentelemetry.io#6726

Open

pichlermarc added 2 commits April 30, 2025 10:53

Merge branch 'main' into trentm-express5

a1fdbbf

chore: sync package-lock.json

343b949

pichlermarc approved these changes Apr 30, 2025

View reviewed changes

wesleytodd approved these changes Apr 30, 2025

View reviewed changes

JamieDanielson approved these changes Apr 30, 2025

View reviewed changes

pichlermarc added the has:owner-approval Approved by Component Owner label Apr 30, 2025

trentm mentioned this pull request Apr 30, 2025

consider bumping the min version of express *tested* #2809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

trentm commented Apr 25, 2025

trentm left a comment

trentm Apr 25, 2025

wesleytodd Apr 30, 2025

JamieDanielson Apr 30, 2025

JamieDanielson Apr 30, 2025

wesleytodd Apr 30, 2025 •

edited

Loading

trentm Apr 30, 2025

wesleytodd Apr 30, 2025

trentm Apr 25, 2025 •

edited

Loading

JamieDanielson Apr 30, 2025

codecov bot commented Apr 25, 2025 •

edited

Loading

trentm commented Apr 25, 2025

pichlermarc commented Apr 30, 2025

pichlermarc commented Apr 30, 2025

wesleytodd left a comment

wesleytodd Apr 30, 2025

JamieDanielson Apr 30, 2025

wesleytodd Apr 30, 2025

wesleytodd Apr 30, 2025

JamieDanielson Apr 30, 2025

wesleytodd Apr 30, 2025 •

edited

Loading

JamieDanielson Apr 30, 2025

trentm Apr 30, 2025

wesleytodd Apr 30, 2025

JamieDanielson left a comment

		const isExpressV5 =
		typeof moduleExports?.Router?.prototype?.route === 'function';

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

Are you sure you want to change the base?

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

Conversation

trentm commented Apr 25, 2025

Testing Notes

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 25, 2025 • edited Loading

Codecov Report

trentm commented Apr 25, 2025

pichlermarc commented Apr 30, 2025

pichlermarc commented Apr 30, 2025

wesleytodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieDanielson left a comment

Choose a reason for hiding this comment

wesleytodd Apr 30, 2025 •

edited

Loading

trentm Apr 25, 2025 •

edited

Loading

codecov bot commented Apr 25, 2025 •

edited

Loading

wesleytodd Apr 30, 2025 •

edited

Loading