feat(dashboards): add legend breakdown below chart#107520
feat(dashboards): add legend breakdown below chart#107520DominikB2014 merged 16 commits intomasterfrom
Conversation
Use index-based matching instead of parsing series names to find matching table rows. Handles both cases: no group by (multiple aggregates in single row) and with group by (single aggregate across multiple rows).
- Filter out "Other" series from the legend breakdown table - Apply TOP_N limit to the table query to match timeseries results - Use filteredIndex to correctly match table rows when there's a group by
|
@cursor review |
Move LegendType from thresholds.tsx to types.tsx where other dashboard types like Widget and DisplayType are defined.
static/app/views/dashboards/utils/prebuiltConfigs/backendOverview/backendOverview.ts
Show resolved
Hide resolved
gggritso
left a comment
There was a problem hiding this comment.
Makes sense overall! I left a bunch of comments here/there, I think most are worth at least thinking about. IMO replacing the styled components with Flex is pretty important and helpful here
…gend-breakdown-below-chart
static/app/views/dashboards/widgetCard/genericWidgetQueries.tsx
Outdated
Show resolved
Hide resolved
gggritso
left a comment
There was a problem hiding this comment.
Nice! The data loading is looking clean 👍🏻 I left a few more comments about smaller issues. Overall this looks good though. Maybe the breakdown table needs a top border, though
static/app/views/dashboards/widgetCard/genericWidgetQueries.tsx
Outdated
Show resolved
Hide resolved
static/app/views/dashboards/widgetCard/genericWidgetQueries.tsx
Outdated
Show resolved
Hide resolved
| const aggregates = [...(query.aggregates ?? [])]; | ||
| const columns = [...(query.columns ?? [])]; |
There was a problem hiding this comment.
I think this is okay, but seems like it'd be good to check this ahead of time. If there's no valid query to copy because it has no aggregates or columns, there's no point trying to make a breakdown table
There was a problem hiding this comment.
Imo, it's probs fine to just handle this at the request layer, i.e if the query is invalid, don't make a series or a breakdown table request at all. If we handle like that, it feels beyond the scope of this PR.
The logic to this is theoretically, every valid series widget can have a legend breakdown table.
| // Table requests require the orderby field to be included in the fields, | ||
| // but series results don't always need that |
There was a problem hiding this comment.
Could you clarify this a bit more? Do you mean that time series widgets may or may not include a query but the table widget must include a query? If that's the case, doesn't the code here need to specify a fallback?
There was a problem hiding this comment.
The series requests automatically add orderby to fields
The table request use eventViewFromWidget, which does not automatically do that. I added a clarifying comment to make this more obvious.
Otherwise we end up with orderby must also be in the selected columns or groupby
| {filteredSeriesWithIndex.map(({series, index}, filteredIndex) => { | ||
| const plottable = plottables[index]; |
There was a problem hiding this comment.
Is it necessary to iterate the series to render this table? Could you iterate the table response results instead? That seems simpler. The table results would also have the meta information for the types and units of the results
There was a problem hiding this comment.
Kinda, but i don't think it simplifies everything. The point of a legend breakdown is we have to have a table row for every single series, no more or no less (perhaps excluding other)
But there isn't a table row for every single series in the no group by and multiple aggregate case, so we still need to do some lookup to map the series aggregate to the table result. We could update this to respect the table meta tho, but the series and table meta should always match for a legend breakdown.
There was a problem hiding this comment.
Ok! I think things will look a little better if/when you iterate through TimeSeries objects instead of the plottables at some point
| queries.push({ | ||
| ...firstQuery, | ||
| fields: [...columns, ...aggregates], | ||
| aggregates, | ||
| columns, | ||
| }); |
There was a problem hiding this comment.
Bug: The time series query and its corresponding breakdown table query can have different orderings because an orderby aggregate is added to the table query but not the series query.
Severity: MEDIUM
Suggested Fix
Ensure that when an orderby aggregate is added to the table query's aggregates list, it is also added to the time series query's aggregates. This will synchronize the data and ordering for both the chart and the table, preventing mismatches.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/dashboards/widgetCard/genericWidgetQueries.tsx#L138-L143
Potential issue: The time series query and the breakdown table query can have different
orderings for the same widget. When a widget's `orderby` field is an aggregate that is
not in its `aggregates` list, the logic in
`createBreakdownTableWidgetFromTimeSeriesWidget` adds this `orderby` aggregate to the
table query's aggregates but not to the time series query's. This causes the two queries
to be based on different sets of aggregates, which can result in different data ordering
between the chart visualization and the breakdown table, leading to a mismatch and
incorrect data being displayed in the table.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
gggritso
left a comment
There was a problem hiding this comment.
LGTM overall. I left a few comments that are worth addressing IMO, but in the end I think could benefit a lot from splitting creation of plottables from conversion of Series to TimeSeries since TimeSeries is so much easier to work with. isOther and yAxis are very helpful when filtering/matching things.
I hope there's a chance to come back to this in the near future and tidy up a bit 🙏🏻
| {filteredSeriesWithIndex.map(({series, index}, filteredIndex) => { | ||
| const plottable = plottables[index]; |
There was a problem hiding this comment.
Ok! I think things will look a little better if/when you iterate through TimeSeries objects instead of the plottables at some point
UI Changes 1. Add new property of a widget, `legendType: default | breakdown` 2. If this property is set to breakdown, the normal legend is not shown, instead the chart is broken down as a table below 3. The breakdown takes 1/3 of the vertical height, it becomes scrollable if there's too many elements <img width="387" height="452" alt="image" src="https://github.com/user-attachments/assets/cdae96e8-d945-4f45-ad54-a59cdaa9a59a" /> Data fetching and logic 1. We create a table widget from the series widget, having the same queries/fields/aggregates 2. We only handle two cases for now a. Case 1: If there is only aggregates and not groupby. we take the first element from the table results (something like `{count(): 123, count_if(): 456}` and look for the aggregate in that element. b. Case 2: If there is only one groupby and one aggregate we iterate over the table results assuming that the order of the line chart results and table (because we have the same orderby in the table and line query). For example if `groupBy = transaction, aggregate = count(), sum()`, the results will be `[{transaction: A, count(): 123, sum():123, transaction: B, count(): 123, sum():123}]` Some minor changes to the backend overview config 1. Apply `breakdown` legend type to backend overview charts 2. Make breakdown charts taller so their table is more visible 3. Correctly sort by time spent
UI Changes 1. Add new property of a widget, `legendType: default | breakdown` 2. If this property is set to breakdown, the normal legend is not shown, instead the chart is broken down as a table below 3. The breakdown takes 1/3 of the vertical height, it becomes scrollable if there's too many elements <img width="387" height="452" alt="image" src="https://github.com/user-attachments/assets/cdae96e8-d945-4f45-ad54-a59cdaa9a59a" /> Data fetching and logic 1. We create a table widget from the series widget, having the same queries/fields/aggregates 2. We only handle two cases for now a. Case 1: If there is only aggregates and not groupby. we take the first element from the table results (something like `{count(): 123, count_if(): 456}` and look for the aggregate in that element. b. Case 2: If there is only one groupby and one aggregate we iterate over the table results assuming that the order of the line chart results and table (because we have the same orderby in the table and line query). For example if `groupBy = transaction, aggregate = count(), sum()`, the results will be `[{transaction: A, count(): 123, sum():123, transaction: B, count(): 123, sum():123}]` Some minor changes to the backend overview config 1. Apply `breakdown` legend type to backend overview charts 2. Make breakdown charts taller so their table is more visible 3. Correctly sort by time spent
UI Changes
legendType: default | breakdownData fetching and logic
a. Case 1: If there is only aggregates and not groupby.
we take the first element from the table results (something like
{count(): 123, count_if(): 456}and look for the aggregate in that element.b. Case 2: If there is only one groupby and one aggregate
we iterate over the table results assuming that the order of the line chart results and table (because we have the same orderby in the table and line query). For example if
groupBy = transaction, aggregate = count(), sum(), the results will be[{transaction: A, count(): 123, sum():123, transaction: B, count(): 123, sum():123}]Some minor changes to the backend overview config
breakdownlegend type to backend overview charts