feat(tracing): Try parameterizing URLs in default routing instrumentation #5411
Conversation
|
This is technically a breaking change. Can we somhow record how many incoming transaction names will be “fixed” by applying this? Also, do we think this needs to happen to all transactions (for ex, if we create a fallback from routing instrumentation, do we apply the heuristic to those names)? |
size-limit report 📦
|
| * Keeps track of what found parameters in the URL evaluate to, e.g: | ||
| * { "uuid-1": "2778216e-b40c-46...", "uuid-2": "86024957-1789-47ec-be7..." } | ||
| */ | ||
| const pathnameParameterValues: Record<string, string> = {}; |
There was a problem hiding this comment.
Question for reviewers: Do you think we actually need to include the parameters somewhere in the transaction data?
I am leaning towards no because we also include originalPathname in the transaction data which is enough for users to figure out what the parameters were.
Transaction data is currently also not queriable in discover so there wouldn't even be an immediate upside to including it in that regard.
There was a problem hiding this comment.
Coming back to this even though the PR is closed (😿)
I think we don't include the parameters, it's un-needed bloat on the event JSON.
|
Closing this because it is breaking (behaviour wise, and also in some hooks we provide top level for processing transactions). Right now we will switch to evaluating whether such a change would be helpful. Might revisit in the future. Good night sweet prince. |
lforst
deleted the
lforst-parameterize-urls-default-routing-instrumentation
branch
Ref: #5342
This PR adds parameterization of URLs in the default browser route instrumentation based on some patterns (numbers, SHA1 hashes, MD hashes, and uuids).
This will not work for all URLs.
14will be identified as a parameter whilesome-random-orgwill not.some-random-orglooks so much like a resource/pathname that we cannot with good conscience parameterize it.This is also the reason why we're keeping the transaction source
'url'for now - simply to avoid cardinality issues downstream.