[@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.

sentry-javascript/packages/tracing/src/integrations/express.ts

Line 201 in 0ddd74f

router[method] = function(...args: unknown[]): void {

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.

[christoph-bessei]

@kamilogorek Any update on this? We can't upgrade because of this issue.

[lobsterkatie]

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:

1. Each router only keeps track of or handles its part of the path.
2. The SDK has access to the routers (both for span creation and the access to the parameterized route) by dint of wrapping .use(), .get(), and the like, which are at their heart registration methods - "when there's a GET request to /xyz, please run this handler" - and which therefore are only run once, at app startup.

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 .use(), .get(), etc which occur after Sentry.init() get wrapped, so the upshot is that only routers the top-most level of the router tree can either create spans or provide a parameterized version of their part of the path. (Further, at the moment, the integration only accepts a single router, the root of the tree, and so nothing below that top level could be wrapped in any case.) The only way to get around this would be to modify the integration to accept multiple routers, and then to create and configure all routers in the same file as the Sentry.init() call, making sure that all .use(), .get(), etc calls happen once the SDK is already running. Gross, and also unrealistic.

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.

[RyanGWU82]

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 /api/users would only report the relative path, and would show up as GET /users. That makes it very confusing to review Sentry error data, as many of our subroutes all displayed as GET /.�

I looked into some workarounds and I found that middleware attached to a router has access to the request's req.baseUrl, which describes where that router is attached. For example, middleware attached to the api route would report req.baseUrl of /api. I can use this to store the baseUrl in my Sentry context, and then I can update the transaction name in the beforeSend hook that the Sentry SDK gives us. (Just attaching the middleware to the top-level of the app doesn't help, because from there the baseUrl is empty. It actually has to be attached to every nested router, to see the baseUrl from that router's point of view.)

So now I'm thinking about monkey-patching express.Router() so that middleware can be applied to every router in our codebase. It would look roughly like this:

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 @sentry/tracing, would it be useful to create a separate ticket for this issue in the error-handling SDK?