Using route instead of url source

[jtmarmon]

Hi there, thanks for building this useful integration!

I'm using Fastify with TRPC, which means my routes all match to a single request handler in fastify under something like /trpc/:rpcCall. To break these routes out by their TRPC function call, I added a getTransactionName:

        getTransactionName: (request: FastifyRequest) => {
            if (request.routerPath === '/trpc/:path') {
                return (request.params as any)['path'];
            }

            return request.routerPath;
        },

This works well, but because the source is type URL, Sentry treats these routes as a high cardinality value and groups them in this <<unparameterized>> block

Happy to submit a fix, but wanted to get your thoughts on a few approaches:

1. Switch the current source value from url to route (here)
It seems to me like fastify's routerPath value should be treated as a route source in sentry and not a url source, since it returns urls like /users/:id and not /users/1. Relevant docs

2. Switch the current value of url to custom if the user provides a getTransactionName function
(self explanatory)
3. Allow users to provide a custom getTransactionSource
Since the docs discourage users from doing this and say source should only be specified by integrations, I'd probably suggest one of the former two.

[dnlup]

Thank you @jtmarmon for the detailed report, let me think about it a little bit. I had some issues with events showing unparameterized when implementing that feature, so your points are very useful.

[dnlup]

It seems to me that the solution 1. is the one to try, but we should also differentiate the case that the url is a plain url, returning source=url , from the case that the url has parameters returning source=route, right? I think I couldn't make this work using route, in the public Sentry instance all the relative transaction would become unparameterized, but it still looks like the logic choice. Would you like to try to open a PR?

[dnlup]

Thinking out loud here. We could add a boolean symbol to our metadata and use the onRoute hook to save true or false if the the route has parameters or not. Fastify doesn't give us that info and checking it at runtime would be less performant and redundant, I think.

[jtmarmon]

@dnlup since the default getTransactionName function (here) uses .routerPath, I'm pretty sure it would never be a plain url. From the fastify docs

routerPath - the path pattern defined for the router that is handling the request

So it would always give you something like "/trpc/:path".

And for routes that don't have any params, e.g. GET /users, referring to /users as a route instead of url is still correct I believe.

[jtmarmon]

E.g in Sentry's express integration, they always use route:

https://github.com/getsentry/sentry-javascript/blob/d33b88c414e27c76006faa7630c4eba226be2107/packages/node/src/requestdata.ts#L67

export function extractPathForTransaction(
  req: PolymorphicRequest,
  options: { path?: boolean; method?: boolean; customRoute?: string } = {},
): [string, TransactionSource] {
  const method = req.method && req.method.toUpperCase();

  let path = '';
  let source: TransactionSource = 'url';

  // Check to see if there's a parameterized route we can use (as there is in Express)
  if (options.customRoute || req.route) {
    path = options.customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`;
    source = 'route';
  }

req.route docs

[dnlup]

I am doing some tests and it looks like the express integration returns url if the url has no parameters (the request is done to /)

I'll get back to you hopefully with some more info 🙏🏻

[jtmarmon]

I can't see your full test, but I think that's because / doesn't match any of the routes that you have there, not because it doesn't have any parameters (e.g. if you hit /m it's a route despite having no parameters, right?)

I think in the case of fastify, you would actually get undefined for routerPath if you were to try to make a request that doesn't match a route. So in the current code you would get POST undefined instead of POST /. Haven't tested it but this issue suggests routerPath is undefined if the request doesn't match any routes fastify/fastify#4050

[dnlup]

Ok, so I made som tests. Here's what I found and is confusing me:

Express

import express from "express";
import * as Sentry from "@sentry/node";
import * as Tracing from "@sentry/tracing";

const DSN = 'your den';
const ENV = 'express-sentry'

const app = express();
Sentry.init({
  dsn: DSN,
  environment: ENV,
  integrations: [
    // enable HTTP calls tracing
    new Sentry.Integrations.Http({ tracing: true }),
    new Tracing.Integrations.Express({
      // to trace all requests to the default router
      app,
      // alternatively, you can specify the routes you want to trace:
      // router: someRouter,
    }),
  ],
  tracesSampleRate: 1.0
})
// The request handler must be the first middleware on the app
app.use(Sentry.Handlers.requestHandler());
app.use(Sentry.Handlers.tracingHandler());
app.use(function fakeUser(request, reply, next) {
  request.user = {
    id: 'some',
    email: 'some@email.com'
  }
  next();
})
app.get('/', function (request, reply) {
  reply.json({ some: 'some' })
})
app.get('/static', function (request, reply) {
  console.log(request.route);
  reply.json({ some: 'static' })
})
app.get('/:param', function (request, reply) {
  console.log(request.route);
  reply.json({ some: request.params.param })
})

app.get('/oops', function (request, reply) {
  throw new Error('Express Error')
})

// The error handler must be before any other error middleware and after all controllers
app.use(Sentry.Handlers.errorHandler());
app.listen(3001);

source is always url as you can see in the debugger.

Fastify

import fastify from "fastify";
import sentry from '@immobiliarelabs/fastify-sentry'

// const DSN = 'https://accef0e7e3e54ec28ddffde850290f42@sentry.pepita.io/15';
const DSN = 'https://d7fbbfd505cd4499843f096a6f12a223@o4504298995515392.ingest.sentry.io/4504298997612544';
const ENV = 'fastify-sentry'

const OPTS = {
  logger: true
}

const app = fastify(OPTS)
app.register(sentry, {
  dsn: DSN,
  environment: ENV,
  tracesSampleRate: 1
})

app.decorateRequest('user', null)
app.addHook('onRequest', function onRequest(request, reply, next) {
  request.user = {
    id: 'some',
    email: 'some@example.com'
  }
  next()
})

app.get('/', async () => {
  return { some: 'some' }
})
app.get('/static', async (request) => {
  return { some: 'static' }
})
app.get('/:param', async (request) => {
  return { some: request.params.param }
})
app.get('/oops', async () => {
  throw new Error('Oops.')
})

app.listen({ port: 3000 })

routerPath is always defined if the route is not 404.

This is something I tested before, that's why I used url as a source. I am still confused though, and the points you make in the issue are still valid to me. Maybe the next step is test a sample of your code (the one you posted at the beginning) and see what differences we have.

[dnlup]

I have some news, I asked for some help about the <<unparameterized>> transactions in the Sentry Discord and received an helpful answer:

(Link: https://discord.com/channels/621778831602221064/777210445442187274/1069925250277576734)

So I think this integration should set source as:

• url when the path is static, like /some/thing
• route when the path has parameters, like /some/thing/:else (it doesn't seem necessary though, but we can do it)

When a custom transaction name is set, the suggestion is that we set the source as custom . I think it's possible to know if we should set url or route in this case too, but I don't have a strong opinion on that and I am ok with custom if it doesn't create the same cardinality problem or any other problem.

With that said, maybe we could also wait I little bit and see if the changes made by the Sentry team are having effects? Are you using a public Sentry instance?

[dnlup]

👋 @jtmarmon Any news? Is this still an issue?