fix(tracemetrics): Uniquely identify multiple series #105610
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
gggritso
left a comment
gggritso
left a comment
There was a problem hiding this comment.
This makes sense overall but I think it needs a follow-up PR where the fix is done at the TimeSeriesWidgetVisualization level (maybe I can help?)
| seriesName: | ||
| multiYAxis && hasGroupings && func | ||
| ? `${formatTimeSeriesLabel(timeSeries)} : ${func.name}(…)` | ||
| : formatTimeSeriesLabel(timeSeries), |
There was a problem hiding this comment.
If I'm reading this right, the overlap is a bug in TimeSeriesWidgetVisualization, no? If TimeSeriesWidgetVisualization gets multiple series with different yAxis but the same groupBy, it'll mash the series together in the legend? If yes, it'd be good to fix that in TimeSeriesWidgetVisualization and then all the UIs would benefit. The caveat is that transformation functions like transformLegacySeriesToPlottables might need to be updated so we don't double-prepend the yAxis
There was a problem hiding this comment.
Yes, I agree. I can look at a follow up to generalize this more! I wanted to keep the possible blast radius of this change smaller until I received some feedback about this approach with metrics and we had some time to consider how it would be implemented for other datasets 🙏
| if (func) { | ||
| timeSeries.yAxis = `${func.name}(${func.arguments[1] ?? '…'})`; | ||
| } |
There was a problem hiding this comment.
Tangentially related, but this feels like maybe it can be part of formatTimeSeriesLabel?
narsaynorath
deleted the
nar/fix/tracemetrics-identify-series-uniquely-with-multiple-yaxes-grouping
branch
When groupings are applied, the current logic just concatenates the grouping names and assumes they're unique series. This does not work well when there are multiple yAxes because both yAxes can have the same labels, but they end up clobbering each other. This detects when multiple yAxes and groupings are applied to render them in a format that more closely resembles the other datasets. i.e. `<concatendated groupings> : <aggregate>` e.g. if we plotted `p50(metric)`, and `p75(metric)` and grouped by platform, you would see a label like `iOS : p50(...)` and `iOS : p75(...)`. I chose to use the ellipsis to format the label because we can only choose a single metric at the moment and this clears up a little UI clutter It may be beneficial in a future PR to do the full label of the first series, just so users know the context without needing to name their widget with the metric or something.
When groupings are applied, the current logic just concatenates the grouping names and assumes they're unique series. This does not work well when there are multiple yAxes because both yAxes can have the same labels, but they end up clobbering each other.
This detects when multiple yAxes and groupings are applied to render them in a format that more closely resembles the other datasets. i.e.
<concatendated groupings> : <aggregate>e.g. if we plotted
p50(metric), andp75(metric)and grouped by platform, you would see a label likeiOS : p50(...)andiOS : p75(...). I chose to use the ellipsis to format the label because we can only choose a single metric at the moment and this clears up a little UI clutterIt may be beneficial in a future PR to do the full label of the first series, just so users know the context without needing to name their widget with the metric or something.