-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(tracemetrics): Improve metrics analytics #104495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This elides metric counts when the metric name is empty, and improves testing around metric panel etc.
| const metricPanelsWithGroupBys = metricQueries | ||
| .filter(mq => !isEmptyTraceMetric(mq.metric)) | ||
| .map(mq => mq.queryParams.groupBys.some((gb: string) => gb.trim().length > 0)).length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: metricPanelsWithGroupBys and metricPanelsWithFilters counts are inflated due to incorrect .map().length usage.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The code incorrectly uses .map().length instead of .filter().length when calculating metricPanelsWithGroupBys and metricPanelsWithFilters. This causes the count to reflect all non-empty metrics, rather than only those that satisfy the specific condition (having group bys or filters). This leads to inflated and inaccurate analytics events for metric_panels_with_group_bys_count and metric_panels_with_filters_count, resulting in silent data corruption in analytics events used to understand user behavior.
💡 Suggested Fix
Replace .map() with .filter() in the counting logic for metricPanelsWithGroupBys and metricPanelsWithFilters to accurately count metrics matching the conditions.
🤖 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/explore/hooks/useAnalytics.tsx#L912-L914
Potential issue: The code incorrectly uses `.map().length` instead of `.filter().length`
when calculating `metricPanelsWithGroupBys` and `metricPanelsWithFilters`. This causes
the count to reflect all non-empty metrics, rather than only those that satisfy the
specific condition (having group bys or filters). This leads to inflated and inaccurate
analytics events for `metric_panels_with_group_bys_count` and
`metric_panels_with_filters_count`, resulting in silent data corruption in analytics
events used to understand user behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5927479
| .map(mq => mq.queryParams.groupBys.some((gb: string) => gb.trim().length > 0)).length; | ||
| const metricPanelsWithFilters = metricQueries | ||
| .filter(mq => !isEmptyTraceMetric(mq.metric)) | ||
| .map(mq => mq.queryParams.query.trim().length > 0).length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrong count for panels with group bys and filters
The metricPanelsWithGroupBys and metricPanelsWithFilters calculations use .map().length instead of .filter().length. The .map() transforms each item to a boolean, but then .length returns the count of all items in the array, not the count of true values. This results in counting all non-empty metric queries rather than only those that actually have group bys or filters.
| table_result_length: resultLengthBox.current, | ||
| table_result_missing_root: 0, | ||
| table_result_mode: 'metric samples', | ||
| table_result_mode: resultMode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Aggregate mode reports wrong table result length
The getAttributes function always uses resultLengthBox.current (samples table length) for table_result_length, regardless of whether resultMode is 'aggregates' or 'metric samples'. When in aggregate mode, it should use aggregatesResultLengthBox.current instead. This causes incorrect analytics data to be reported for the aggregates table result length.
| table_result_length: resultLengthBox.current, | ||
| table_result_missing_root: 0, | ||
| table_result_mode: 'metric samples', | ||
| table_result_mode: resultMode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Aggregate mode reports wrong table result sort
The getAttributes function always uses formattedSortBysBox.current (samples sort) for table_result_sort, regardless of whether resultMode is 'aggregates' or 'metric samples'. When in aggregate mode, it should use formattedAggregateSortBysBox.current instead. The formattedAggregateSortBysBox is defined and included in the aggregates useEffect dependency array but is never actually used in the analytics payload.
This elides metric counts when the metric name is empty, and improves testing around metric panel etc.