-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[@sentry/tracing] Nested express custom middlewares detection #3155
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
Comments
I provided a sample API project via Sentry Support, ticket #41043. It's for a similar issue, and would likely help with this bug as well. |
Unfortunately, it's currently not supported due to Express limitations. The frameworks itself doesn't allow for traversing nested routers, nor it provides any way to get this data other than overriding Additionally (@adamup928 this is in regards to your support ticket), it's not possible to get a generic nested route either, at least not with 100% guarantee.
We'll need to think of a completely different solution here. Any feedback appreciated. |
Thanks, @kamilogorek - I understand the issue and see the difficulty in creating a fix. Unfortunately, the new route handling introduced in 5.28.0 makes Sentry's Performance feature a poor experience for an API with nested routers. We really can't update until this is fixed or the old behavior becomes an option. Regarding the 2 flaws in getting a generic nested route: I believe if it was documented well, we would be willing to add |
@adamup928 agreed, I'll make sure that this issue gets addressed as soon as possible. |
@kamilogorek Any update on this? We can't upgrade because of this issue. |
Hi, all. I took some time to look into this, and unfortunately, Kamil is right - without a near-total rewrite of the Express integration, there's no good way to solve this problem. I think I might have a hack which could solve part of it (the labeling of transactions, accounting for the substitution problem Kamil mentioned), and it's conceivable a similar method could work to solve the original issue mentioned here (that of missing spans), but to really make it work, we'll have to adopt a different approach. That's not off the table, but moves this out of the category of "small bug fix" and into the category of "work that has to be planned for," which is why it hasn't happened yet, unfortunately. A little more detail, in case it's helpful: The first things to know are:
The root cause of the problem is that by the time the SDK comes along to do its wrapping, all but the top-level routers have already registered their handlers: as soon as you import a router, its code runs and all of the sub-routers get registered. But of course, you have to import the router in order to be able to give it to the SDK to wrap, so by definition that will happen before the SDK can do its thing. Only calls to I spent more time than I care to admit stepping through Express routing code, and I think there may be a spot we could hook into, as the parameters are being processed, which would let us reconstruct each part of the parameterized path as it's matched. (It's similar to the substitution Kamil mentioned, but with the ability to disambiguate identical parameter names, as well as parameter values which match hard-coded parts of the path.) I haven't had a chance to fully test it out, though, nor to see if similar hackery could be employed for span creation. Ultimately, I think our better bet will be to continue to accept only the top-level router, but to then actually walk the router tree after it's fully formed. I know this doesn't solve the problem, but hopefully it at least gives you context for why it hasn't been solved yet. I'll update here as anything changes. |
On a related topic, my company also uses a lot of nested routers in our Express apps. The nesting causes problems for our Sentry error logging, specifically in the transaction name field. From the example that Pierre posted initially, a route like I looked into some workarounds and I found that middleware attached to a router has access to the request's So now I'm thinking about monkey-patching const express = require('express');
const RouterClass = express.Router;
express.Router = () => {
const router = RouterClass();
router.use((req, res, next) => {
Sentry.setContext('baseUrl', req.baseUrl)
return next();
});
return router;
} This feels awfully hacky but it works. Anyone have thoughts, or a better idea? @kamilogorek, since this ticket was originally tagged |
Package + Version
@sentry/tracing
5.29.2Description
At first I had the problem described in #2968 but I saw #2972 and added the methods I wanted to log:
This works great, except that it doesn't seem to handle nested middlewares.
I checked the code source and indeed it seems the wrapping only applies to the
app
orrouter
parameter.sentry-javascript/packages/tracing/src/integrations/express.ts
Line 201 in 0ddd74f
That means considering a code like this:
/test
we'll indeed getmiddleware.get
corresponding to our first endpoint in the list of spans./api/users
we'll only getmiddleware.use router
corresponding to the nested router, but the second endpoint will not appear at all in the list of spans.The text was updated successfully, but these errors were encountered: