Repository navigation
Routes with query parameters got wrong transaction name #6586
Description
Activity
Hi, thanks for writing in! Can you share how you set up the SDK? Thanks!
Hi, @lforst! Thanks for quick reaction.
// some imports import * as Sentry from "@sentry/node"; import * as Tracing from "@sentry/tracing"; const routes = new Router(); // ... routes.use('/api/affiliate', affiliateRouter); routes.use('/api/accounts', accountsRouter); // ... routes.use('/api', fileRouter); // ... export const app = express() .disable('x-powered-by') .disable('etag') .use(cors()) .use(cookieParser()) .use(loggerMiddleware) .use(bodyParser.urlencoded({ extended: true, limit: '20mb', })) .use(bodyParser.json({ limit: '20mb', })); Sentry.init({ tracesSampleRate: 0.2, integrations: [ new Tracing.Integrations.Prisma({ client: prisma }), new Sentry.Integrations.Http({tracing: true}), new Tracing.Integrations.Express({app}), ] }); app .use(Sentry.Handlers.requestHandler({ user: ["id"] })) .use(Sentry.Handlers.tracingHandler()) .use(passport.initialize()) .use(routes) .use(function NotFoundHandler(req, res, next) { res.sendStatus(404); }) .use(Sentry.Handlers.errorHandler()) .use(...) // validation errors serialisation
I have the same issue and posted a minimal reproducible example in the above PR.
I believe the nested router wasn't well supported in Sentry.
Right now my temporary fix is to build a map that contains a path regex test string to their transaction name. Then manually set the transaction name by testing all the regex one by one.
@langovoi, thanks for reporting this.
I have tried to reproduce the issue based on the example you provided.
In summary
Sending a GET request to the route with query parameters:
http://localhost:3500/api/file/v2/15cf8416-5d66-4cbb-b12f-37361d3c0116?type=48x32To server:
const express = require("express"); const cors = require("cors"); var app = express(); const Sentry = require("@sentry/node"); const Tracing = require("@sentry/tracing"); Sentry.init({ dsn: "__DSN__", release: "1.0", integrations: [ new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app }), ], tracesSampleRate: 1.0, }); app.use(Sentry.Handlers.requestHandler()).use(Sentry.Handlers.tracingHandler()); app.use(cors()); const routes = express.Router(); const accountsRouter = express.Router(); const fileRouter = express.Router(); fileRouter.get("/file/v2/:uuid", function (_req, res) { res.send("Success"); }); routes.use("/api/accounts", accountsRouter); routes.use("/api", fileRouter); app.use(routes); app.use(Sentry.Handlers.errorHandler()); app.listen(3500, () => { console.log("Server started on port 3500"); });
The transaction seems to be parameterized correctly.
Is there anything I'm missing here? Or, could you provide a minimal repro case please?@brotherko, I could reproduce your case, but not sure if it's the same issue.
Copied here from #5450 (comment) for visibility .
const route1 = Router(); const route2 = Router(); route1.use('/route2', route2) route2.use('/:param1', (req, res) => res.send(`hello ${req.params.param1}`)) app.use('/route1', route1)
When I browse => route1/route2/someparameter
Sentry isn't able to capture the transaction as /route1/route2/:param1 but as route1/route2/someparameter
The difference is that you're using
route2.use('/:param1', ...)to handle incoming requests, and Sentry seems to fail to parameterize the route (with or without query parameters) in that case.But it works if I replace it with
route2.get('/:param1', ...). I don't have much context ifroute.useis a valid replacement toroute.[method], and if so, should we instrument it?@Lms24 might have more context on that.
Do you think the decision not to parameterize middleware is intended or not?
I take a deeper dive into the express codebase. I understand how middleware processes the path a little bit differently compared to using the method keywords(GET/POST/PATCH...). I'm happy to send over a PR but just want to make sure if we want to instrument it in the first place (In fact I can't think of why we don't)
Hi @brotherko, since you took a look at the express source code: Am I guessing correctly, that
usedoesn't invoke theprocess_paramsfunction which we patch to parameterize the taken route?I think overall, we'd definitely like to instrument middleware. In fact we tried to do this but iirc, the problem was that this only worked for middleware that was evaluated after this function was called. So for every middleware that was imported (i.e. evaluated before Sentry.init was called), this didn't work.
We're quite busy around a lot of projects at the moment, so if you have some time, we'd greatly appreciate a PR. Thanks!
5 remaining items
@erickalmeidaptc Route parameterization is something the SDKs are responsible for. Since the nestjs integration you're refering to is not maintained by us I recommend you raise an issue in their repository. Feel free to let the mainteainers know that we are here for any guidance they might need!
We have the same issue. Stepping though the code my guess is that the issue is this:
sentry-javascript/packages/tracing-internal/src/node/integrations/express.ts
Lines 337 to 339 in ad3547d
In combination with
extractPathForTransaction()setting thecustomRoutewithout callingstripUrlQueryAndFragment.If I understand it correctly
originalUrlwill always be exactly the URL passed to the server (I.e. with query params and fragments) and since query params aren't defined in the routerreq._hasParameterswill be false.Defining a route using
app.get('/route')which is then called with/route?param=foowill cause this issue but defining it usingapp.get('/route/:someParam')and calling/route/foo?param=foowill work.I think a reasonable solution would be to call
stripUrlQueryAndFragment()after having retrieved theoriginalUrlin the code I linked above. I'm happy to submit a PR if you think that's a good approach?For now we have worked around it by adding a middleware that runs the following on the response:
res.once('finish', () => { transaction.setName(stripUrlQueryAndFragment(transaction.name)); });
You're right, I think we should still strip params and fragment in this case.
Do you want to open a PR for this? Should be fairly easy to do (tests might need some adjustments but we can take a look at this then). (Feel free to say no, I was just thinking it might be a low hanging fruit :) )
Absolutely, I'll take a look tomorrow. Thanks for the feedback!
Reacted by Lukas Stracke- added 5 commits that reference this issue
on May 25, 2023 - added a commit that references this issue
on May 30, 2023
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which package are you using?
@sentry/node
SDK Version
7.27.0
Framework Version
Express 4.17.2
Link to Sentry event
No response
Steps to Reproduce
Expected Result
Get transaction name
GET /file/v2/:uuidActual Result