feat(instrumentation-express): Add support for Express v5 #2437
Conversation
Codecov ReportAttention: Patch coverage is
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
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
"@types/express": "4.17.21"
There is now a v5 for the typing too
https://www.npmjs.com/package/@types/express
|
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 |
There was a problem hiding this comment.
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.
| moduleExports => { | ||
| const routerProto = moduleExports.Router as unknown as express.Router; | ||
| const isExpressV5 = 'Route' in moduleExports.Router; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah agreed. I suspect it might be a better idea to target the two different versions separately 🤔
|
Sorry should have fully read the PR description before posting a review.
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 |
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }); | ||
|
|
||
| app.get('/post/:id', (req, res) => { | ||
| res.send(`Post id: ${req.params.id}`); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
There was a problem hiding this comment.
I think here the "user provided value" is stringified, so there's no security risk, right ?
| app.use('/double-slashes/', router); | ||
| router.get('/:id', (req, res) => { | ||
| setImmediate(() => { | ||
| res.status(200).end(req.params.id); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
There was a problem hiding this comment.
Hello,
It seems that here a good way to remove the error, should be to force send a string using string literals:
| res.status(200).end(req.params.id); | |
| res.status(200).end(`${req.params.id}`); |
|
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 |
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 |
The goal is for Express to eventually support publishing events to `diagnostics_channel` (#15107) so that Open Telemetry can instrument it without monkey-patching internal code. However, this might take a while and it would be great to support Express v5 now. This PR is a stop-gap solution until that work is complete and published. This PR vendors the code added in my otel PR: - open-telemetry/opentelemetry-js-contrib#2437 - Adds a new instrumentation specifically for hooking express v5 - Copies the Express v4 integration tests to test v5 - The only changes in the tests is the removal of a couple of complex regex tests where the regexes are no longer supported by Express. - Modifies the NestJs v11 tests which now support full Express spans
|
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 |
|
With v5 moving to |
|
This can probably just ship as it is and we can migrate to a 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. |
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
|
Closing in favour of: |
|
@timfish Thanks for doing this.
Did you forgot to push changes to package.json that included showing running test/v5 tests? In any case, I opened an alternative PR (#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. :) |
Sorry I can't find anything!
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. 🙌 |
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:
middleware - queryandmiddleware - expressInitspans are missing, no doubt due to reworking of the internals.*paths. Instead/.*/regex should be used.'/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.