getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
579 stars 206 forks source link

Extract route on ASP.NET for Performance Transaction Name #2603

Open thinkocapo opened 1 year ago

thinkocapo commented 1 year ago

Problem Statement

The customer wants to change the performance transaction grouping The transactions are all grouped together into GET /*//*.

But transactions value shown in the JSON are /api/documents. ["transaction","GET //"]],"_meta":{"transaction":{"":{"rem":[["GET ///**","s"]],"val":"GET /api/contracts"}}.

Other developer commentary:

"This is server side bucketing, their transaction have source URL....not sure how they instrumented their app, but the behaviour is “correct” if transaction_info.source is URL (which should be the default)..... ....hardcoded namesource as URL ...Well, this is not wrong. ...Can we extract something like a route in ASP.NET? ... Like /user/edit/1 becomes /user/edit/{id} ...If this is not possible, source URL is correct Yeah, we don’t have anything in place there to get the route. ...Supposedly we should be able to get there tho by trying to read from the routetable and if that doesn’t work fall back on the url.. but I’d have to give that a try first. ...Afaik there is nothing broken here. This is “behaving as intended”. ...The ASP.NET middleware puts the namesource as URL ...So this turns into a feature request."

Solution Brainstorm

Don't group them all together as GET /*/*.

bitsandfoxes commented 1 year ago

To give some context here: Our ASP.NET HttpContextExtension sets the transaction's NameSource to URL and does not extract the route.

jamescrosswell commented 11 months ago

Additional Context

It's worth noting we might have some challenges with the default route (which is often the only route used by many applications) which is:

        routes.MapRoute(
            name: "Default",
            url: "{controller}/{action}/{id}",
            defaults: new { controller = "Home", action = "Index", id = "" },
            constraints: new { id = "\\d+" }
        );

Literally everything maps to "{controller}/{action}/{id}" as the routename when using this convention based routing configuration. We could assume that the controller name and action name are safe to share (i.e. not PII) and so pass those through but leave the id masked... which might give us more useful groupings if people are using the default route.

When people are not using the default route it's really hard to know which parts of the route are safe to store in the SentryEvents and which bits are potentially PII though... so we'd probably just have to leave everything masked in those cases (i.e. everything sharing the same route, where it's not the default route, would be grouped by Sentry on the server).

At least, that's one possible implementation.

bruno-garcia commented 11 months ago

Relates to: https://github.com/getsentry/sentry-docs/pull/7866