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