ref(browser): Move navigation span descriptions into op #13527
Conversation
size-limit report 📦
|
0Calories
requested review from
lforst,
a team and
andreiborza
and removed request for
a team
| op: 'browser', | ||
| name: name || event, | ||
| op: `browser.${name || event}`, | ||
| name: entry.name, |
There was a problem hiding this comment.
m: assigning the URL here makes the span name become high-cardinality. Are we aware of this? Does it have impact on Relay/grouping/etc? These are unparameterized, raw URLs. I assume they not only contain path parameters but also potentially query and hash params. We generally tried to stay as far away as possible from this because historically, it always led to problems (e.g. dynamic sampling consistency, transaction name grouping)
The raw urls can also contain all kinds of tokens and ids.
I'm not 100% against changing this, I just want to make sure we're aware of the implications.
I guess an alternative would be to just describe a bit what this span is doing?
There was a problem hiding this comment.
Hi Lukas, a very valid point but it's something we are aware of! There's currently no grouping in Relay for browser spans, so the cardinality will not be an issue. However, we already have the functionality available to scrub/parameterize URLs, which we use for resource and HTTP spans, for example. My next step is to update Relay to scrub these browser spans so they can be grouped safely.
Putting something different and low cardinality in the description would not be as useful, since we'd be losing a lot of details when looking at the span summary.
|
@JonasBa In case this lands we need to update the trace gymnastics to also work with |
|
Good thinking @lforst, let me open a PR to support this on sentry. |
There was a problem hiding this comment.
Sorry for the wait @0Calories! I think technically this is ready to go (so I'll ✅ it) but before we merge this: @JonasBa can you give us a timeline on the necessary trace view change? Ideally we make this a seamless transition.
|
Sounds good! Not in a rush so I will wait for the trace view change before merging this |
|
@lforst @0Calories sorry, I was out last week. I'll make the changes today to handle this change |
Add reparenting support for the incoming renaming change in getsentry/sentry-javascript#13527
Add reparenting support for the incoming renaming change in getsentry/sentry-javascript#13527
|
I guess we can merge this now? |
|
Before I forget: Can you prepare a changelog entry in the |
|
Thanks for the reminder! Let me do that now |
Makes some modifications to
browserspans:browserspans into the op, e.g.browser - cache->browser.cacheperformanceEntryobjects'names, in this context it is the URL of the pageThis change is being made so that these
browserspans can be ingested and grouped. Currently, all browser spans are grouped into a singularbrowserspan, despite each of the operations that these span represent doing very different things.This will improve the experience in the Spans tab of transaction summary and span summary, since we will be able to have proper groupings for
browserspans.