Improve Express Support

[Lms24]

Background

In this (meta) Issue, we would like to summarize and categorize different opportunities to improve Sentry support for Express. Most things are related to Sentry's performance monitoring product but in case we come across improvement opportunities for error monitoring, we're more than happy to collect them here.

Improvement Opportunities

The following sections list possible improvements for Express

Transaction names and URL Parameterization

We recently improved URL parameterization in #5450. Briefly summarized, we patch the Express router prototype to get to a fully parameterized route before that route's handler callback is executed. This solution isn't perfect (there are still timing issues) but it's probably the best we can do for now.

(In theory, we could explore building a tree of all possible routes and then matching incoming request URLs against this tree (with Express's matching strategies), but IMO we shouldn't open this box as it has a lot of potential to turn into an edge case nightmare without any guarantee for success. Possibly related: #5050)

Parameterization termination criterion

One thing we could look into, would be a detail in our current router instrumentation: The criterium that determines when we have reached the "last" partial route and hence the full parameterized route was found. We currently do this by comparing the number of URL segments in the incoming vs. the accumulated parameterized route. This approach clearly has its limitations (e.g. it being brittle with regex and array routes).

• [Research] is it possible to determine the final route layer more reliably

Transaction names of array routes

If users currently have a route defined with an array of paths (which can be strings or regexes (mixed)), like so:

route.get(['path', 'other_path/:id', /regex_path/], (req, res, next) => {...})

The resulting transaction names they get will be a concatenation of all items in the array:

GET /path,/other_path/:id,/regex_path/

One could argue that this is not a nice transaction name because it is not clear which item of the array actually matched the incoming array. Instead, a transaction name like so might be better:

GET /other_path/:id

For more info and explanations see #5489

• [Discussion] Is it preferrable to only take the part of the array that was matched as a transaction name?
  • If yes, Change the transaction name (probably not trivial)

It's important to note that a change here does not improve transaction name cardinality. In fact, it would slightly increase the number of transactions because we would create separate (but still parameterized) transactions for each item in the routes array.

Express 5

Express 5 has been in beta for quite some time but we were recently notified that our new router instrumentation caused problems there (see #5501). For now we simply disabled early URL parameterization for the case that we don't encounter Express 4. Besides adapting our router instrumentation in the future for Express 5, we should investigate what else needs to be done to support Express 5.
For more information, see #5501

• [Research] Investigate Changes in Express 5 more thoroughly
  • Is the new route matching syntax problematic for our instrumentation?
  • How can we access and instrument the new Router?
  • Are errors reported correctly?
• Make routing instrumentation/early URL parameterization compatible with Express 5
• Make rest of Sentry product compatible with Express 5 if it isn't

Span Creation in Nested Routers

Due to a timing conflict when nested routers are initialized vs. when Sentry is initialized, we mostly fail to instrument the registration methods (router.get(...)) of nested routers. These methods are usually executed when the nested router is imported, which happens before Sentry.init is called. This leads to missing spans of the child routers because we do not instrument the callbacks.

• [Research] Can this be improved?
  • Maybe a similar approach of patching prototypes like we did with the router?

More context: #3155

Proper @sentry/express package

We've discussed creating a @sentry/express package that builds on top of @sentry/node. Currently, our Express-specific support is limited to the Express integration users add to the Node SDK. Transactions are e.g. created in the Node SDK (in the tracingHandler middleware) and middleware spans + transaction parameterization happens in the Express integration.

This has already been requested by the community: #4700

• [Discussion] is it worth to do this?

Potential pros/cons:
+ Signals proper Express support (similarly to what we intend to do for Svelte)
+ Gives us the possibility to add more express-specific stuff without cluttering (or also influencing) the Node SDK used in other frameworks
+ Might help us to declutter the Node SDK/define more clearly what's Express-specific and what was just added on top
+ Lets us declare express versions as peer dependencies
+ Gives us the possibility to track Express SDK vs. rest of Node SDK usage
- What about other frameworks that build on top or are similar to Express (Koa, Connect)
- Are there dependencies between Express stuff and NextJs/Serverless?
~ We'd probably need to de-unify types (e.g. CrossplatformRequest) which might lead to a small portion of duplicated code. This, however, might not be too bad (or worth the sacrifice) considering that the type is problematic atm (#5768)

Misc; Bugfixes

• Cookies are being sent in an error-report by default in Express app #5458
• Sentry Express instrumenting whole app instead of only router #5433
• Wrong transaction name if multiple routers match #6004

Call for Feedback

As always, we'd appreciate feedback! Have you been using our Node SDK (with Express) and you would like to see something changed or some feature implemented? Please let us know! Also, which of the items listed above would be most important to you?

Please feel free to give this post a 👍 if you would like to see us dedicating time to improving Express. Community feedback helps us prioritizing what to work on next.

[TubbyStubby]

Hi, I have been looking at different APMs lately for work and finally decided to settle with sentry. Was having issues with transaction names, saw a couple of issues and finally got to this discussion. Mostly the issue was with nested routers and pretty much our whole server is built using those.

I checked out quite a number of issues mentioned in the above post, saw a bunch of articles and looked into some other projects and finally decided on patching express. I think this reply is closest to what ended up doing and this seems like the best option to go with in my opinion.

So, basically, I have patched application and Router to collect path info in req and just before request ends, I am updating the transaction name, no I am not doing it manually at the end of each request rather I wrapped send method to do it. There are few edge cases in doing so, like for errors I have to add an error middleware before Sentry.Handlers.errorHandler to update the name and handle 404s in similar way.

I have tested it with node18, express v4.18.2, and sentry v7.35.0. Haven't faced any issues as of yet. I haven't fixed the array issue in this right now but should be doable, I guess.

I have put the express patcher in a package if anyone wants to check it out - px5 (it's quite messy 😅).

Hoping something like this is added to the agent soon.

[Lms24]

Hey @TubbyStubby, thanks for sharing this! I took a brief look and I like your approach. Seems more robust than what we currently do. I've shared your message with the rest of the team. Let's see if we can explore this direction when we take a look at express. (oh, we'll be sure to give proper attribution if we end up with something inspired by your solution, no worries ;) )