Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

express v5 beta support #1249

Closed
wants to merge 6 commits into from
Closed

express v5 beta support #1249

wants to merge 6 commits into from

Conversation

aryamohanan
Copy link
Contributor

@aryamohanan aryamohanan commented Jul 29, 2024

Express has been preparing for a major release (v5) for some time now, and they have already released several beta versions, including 5.0.0-beta.3.

For more details on migrating to Express v5, please refer to the migration guide.

This PR does not consider any of the breaking changes listed; it simply updates the version to the latest beta to see our tests

This PR is intended to test if our current setup works correctly with the latest beta version 5.0.0-beta.3 and to analyze the changes required for updating to v5.

At present, the npm install fails when we directly update Express to the beta version in our repo, due to the dependency with apollo-server-express package. To address this, I have kept the existing Express version and installed the beta version alongside it as express-beta.

Impact on our Tests

Issue Possible Causes Explanation
1. Missing Express middleware otel spans see - Middleware initialization was removed in commit 78e5054.
- Middleware query functionality was removed in commit dcc4eaa.
Middleware setup changes in Express have resulted in missing middleware spans.
2. Request body initialized as undefined instead of {} see - req.body is no longer always initialized to {} as seen in commit af341b0. The request body initialization change results in req.body being undefined rather than {}.
3. http.error is not traced see - The router module was used for routing instead of the previous implementation in Express, as seen in commit cec5780. Changes in routing implementation have affected error tracing.
4. app.all('*') is not supported - (*) is no longer valid and must be written as (.*), for example app.all('(.*)'. Changes in express implementation require updating wildcard routes to use (.*) instead of *. Special * path segment behavior removed. see expressjs/express@1574925

Tasks

  • Analyse all the issues with express 5 beta
  • Support express 5 beta, see the PR

Post-Release Considerations for v5

  • Add express v5 support when it comes out
  • Consider all test case fixes( refer the comments in this PR)
  • Remove beta version support
  • Update the existing test env to use express v5.

Refs
INSTA-12542
INSTA-12541
Track express v5 updates in expressjs/discussions#233

@@ -146,6 +146,7 @@
"eslint-plugin-mocha": "^10.1.0",
"eslint-plugin-monorepo-cop": "^1.0.2",
"express": "^4.19.2",
"express-beta": "npm:express@^5.0.0-beta.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We encountered an ERESOLVE error while resolving [email protected] due to a version conflict with Express. Specifically:

The version of Express required by [email protected] conflicts with the beta version of Express we are using. To address this, I am using express-beta to avoid the dependency conflict.

@aryamohanan aryamohanan removed the WIP label Jul 31, 2024
@aryamohanan aryamohanan changed the title test: updated express to latest beta version test: express v5 beta support Aug 2, 2024
@@ -74,7 +74,7 @@ app.delete('/received/spans', (req, res) => {

// With the exception of /heartbeat, the Lambda extension would forward all requests to the
// back end (serverless-acceptor). This handler mimicks that behavior.
app.all('*', (req, res) => {
app.all('(.*)', (req, res) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Express 5 beta, wildcard routes have been changed to use (.*) instead of *. For more details, see the migration guide and the commit on GitHub.

shimmer.wrap(express.Router, 'handle', shimExpress4Handle);
shimmer.wrap(express.Router, 'use', shimExpress4Use);
}

if (express.Router && express.Router.prototype) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The routing logic has been refactored to use the router module instead of a locally defined one. The methods to capture uncaught errors are now on the prototype of express.Router. This ensures the code correctly wraps and intercepts the handle and use methods in Express 5 beta for error capturing.
reference expressjs/express@cec5780

'incoming-request': {
body: req.body
body: req.body ? req.body : {}
Copy link
Contributor Author

@aryamohanan aryamohanan Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The req.body is no longer always initialized to {} as seen in commit af341b0
Here we are expecting an object in the test case, earlier we are getting an empty object but now its undefined.

@@ -210,7 +210,8 @@ function verifySpans(spans, appControls) {
// EXIT www.example.com
// 2 x express middleware, 1 x request handler
// 1 x tlc connect, 1 x tls connect
expectExactlyNMatching(spans, 6, [
// TODO: middleware spans are not collected when migrating to express v5 beta.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Middleware initialization was removed in commit 78e5054.
  • Middleware query functionality was removed in commit dcc4eaa.
    The otel middleware spans are missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats I guess because Opentelemetry does not support Express v5 beta yet or?

Copy link
Contributor Author

@aryamohanan aryamohanan Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are not yet added support for express v5 beta, see supported versions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay as the exporter is getting deprecated anyway, we don't care that much.

@aryamohanan aryamohanan changed the title test: express v5 beta support express v5 beta support Aug 6, 2024
@aryamohanan
Copy link
Contributor Author

Closing this PR, as https://github.com/instana/nodejs/pull/1261/files already merged.
Can use this PR as a reference to INSTA-12542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants