Sentry-javascript: [@sentry/tracing] Nested express custom middlewares detection

[Pierre-Demessence]

Package + Version

@sentry/tracing 5.29.2

Description

At first I had the problem described in #2968 but I saw #2972 and added the methods I wanted to log:

new Tracing.Integrations.Express({ app, methods: ['get', 'post'] }),

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 or router parameter. https://github.com/getsentry/sentry-javascript/blob/0ddd74f03035c9b4cbbaf8cdd5e7e77fbd7fda46/packages/tracing/src/integrations/express.ts#L201

That means considering a code like this:

// index.ts
const app = express();

Sentry.init({
  dsn: "...",
  integrations: [
    new Tracing.Integrations.Express({ app, methods: ["get"] }),
  ],
  tracesSampleRate: 1.0,
});

app.get("/test", someHandler); // first endpoint

app.use("/api", require("./api")); // nested router


// api.ts
const router = express.Router();

router.get("/users", someHandler); // second endpoint

export default router;

• If we call /test we'll indeed get middleware.get corresponding to our first endpoint in the list of spans.
• If we call /api/users we'll only get middleware.use router corresponding to the nested router, but the second endpoint will not appear at all in the list of spans.

[adamup928]

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.

[kamilogorek]

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 use/[method] prototype methods and storing this data ourselves somewhere in the memory.

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.
The problem is that, Express is not giving us this data here either. So in order to get it, we'd need to produce a generic route by parsing originalUrl and applying req.params with a naive search/replace. eg. /foo/42/bar with req.params = { id: 42 } could produce /foo/:id/bar.
However, this has 2 big flaws:
1) Child route always win, which means that if there are 2 params named :id, we'll have invalid substitutions, eg. /user/:id/album/:id cannot have req.params = { id: 42, id: 1337 }
2) Every nested router, has to use mergeParams: true options in order to make it even work.

We'll need to think of a completely different solution here. Any feedback appreciated.

[adamup928]

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 mergeParams and ensure that identically-named parameters don't exist in our routes. The search-and-replace logic inside Sentry could also detect a duplicate parameter name and add a prefix to distinguish between them. If I think of any alternatives, I'll be sure to let you know.

[kamilogorek]

@adamup928 agreed, I'll make sure that this issue gets addressed as soon as possible.