Routes with query parameters got wrong transaction name

[langovoi]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

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

1. Create express route with params

fileRouter.get('/file/v2/:uuid'...

2. Use this route with query parameters

GET /api/file/v2/15cf8416-5d66-4cbb-b12f-37361d3c0116?type=48x32

Expected Result

Get transaction name GET /file/v2/:uuid

Actual Result

[lforst]

Hi, thanks for writing in! Can you share how you set up the SDK? Thanks!

[langovoi]

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

[brotherko]

#5450 (comment)

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.

[onurtemizkan]

@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=48x32

To 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?

[onurtemizkan]

@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 if route.use is a valid replacement to route.[method], and if so, should we instrument it?

@Lms24 might have more context on that.

[brotherko]

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)

[Lms24]

Hi @brotherko, since you took a look at the express source code: Am I guessing correctly, that use doesn't invoke the process_params function 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!

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[erickalmeidaptc]

I have the same issue.
I'm using NestJS framework integrated with Sentry using nestjs-sentry. The route params are not being correctly grouped.

Expected: GET /interaction/:uid/:lang/persist
Recorded on sentry: GET /interaction/QiFLKQen0iZOH6NvDmtPx/en-ng/persist

[lforst]

@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!

[richardsimko]

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

	if (req._reconstructedRoute !== req.originalUrl) {
	req._reconstructedRoute = req.originalUrl;
	}

In combination with extractPathForTransaction() setting the customRoute without calling stripUrlQueryAndFragment.

If I understand it correctly originalUrl will 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 router req._hasParameters will be false.

Defining a route using app.get('/route') which is then called with /route?param=foo will cause this issue but defining it using app.get('/route/:someParam') and calling /route/foo?param=foo will work.

I think a reasonable solution would be to call stripUrlQueryAndFragment() after having retrieved the originalUrl in 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));
});

[Lms24]

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 :) )

[richardsimko]

Absolutely, I'll take a look tomorrow. Thanks for the feedback!