fix(tracemetrics): Read equation for total count in widget#116973
Conversation
| } | ||
| } | ||
|
|
||
| if (filters.length === 0) { |
There was a problem hiding this comment.
Bug: The code handles a combined state with functionAggregates and equationAggregates, but this state is explicitly documented as not intended and lacks any test coverage.
Severity: LOW
Suggested Fix
Add a test case that validates the behavior when both functionAggregates and equationAggregates are present to ensure the combined filter logic is robust. Alternatively, if the constraint is strict, add runtime validation to enforce it and throw an error or log a warning if both are detected.
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/hooks/useWidgetRawCounts.tsx#L69
Potential issue: The code combines filters from `functionAggregates` and
`equationAggregates` using an `OR` join. While the current filter generation logic
correctly parenthesizes sub-queries, making the combined query functional, a design
comment explicitly states that only one type of aggregate should be present at a time.
The code does not enforce this constraint, and there are no tests for the scenario where
both are present. This creates a risk that if future UI or data changes allow both
aggregate types in a single widget, the resulting behavior will be completely untested
and could lead to unexpected outcomes.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
It's fine that it handles both. The filters that are added are self contained and won't interact oddly with each other. Plotting aggregates and equations just isn't something we support with the UI at the moment.
Process equations to get their total count as well for the confidence footer in dashboard widgets. We were gathering the filter with
extractTraceMetricFromColumnbut this only handles functions. We need to add a branch to handle equations using the helper. There's a chance I can combine the functionality of these, but for now I will push this to patch the behaviour.