feat(tracemetrics): Update table implementations and fix metric name #102737
Conversation
Splits out tables and cleans up some code before infinite table + trace view changes.
github-actions
bot
added
the
Scope: Frontend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| embedded={embedded} | ||
| > | ||
| {isValueColumn ? ( | ||
| <Tooltip showOnlyOnOverflow title={row.value}> |
There was a problem hiding this comment.
Bug: row.value is incorrectly used for tooltip title; it should be row[TraceMetricKnownFieldKey.METRIC_VALUE].
Severity: HIGH | Confidence: 1.00
🔍 Detailed Analysis
In the SampleTableRow component, line 293 attempts to access a tooltip title using row.value. However, the TraceMetricEventsResponseItem type definition specifies the metric value as row[TraceMetricKnownFieldKey.METRIC_VALUE]. This incorrect property access pattern is inconsistent with other parts of the codebase, such as line 88 in the same file, and will result in the tooltip displaying undefined instead of the actual numeric metric value.
💡 Suggested Fix
Change title={row.value} to title={row[TraceMetricKnownFieldKey.METRIC_VALUE]} on line 293 in metricsSamplesTableRow.tsx.
🤖 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/metrics/metricInfoTabs/metricsSamplesTableRow.tsx#L293
Potential issue: In the `SampleTableRow` component, line 293 attempts to access a
tooltip title using `row.value`. However, the `TraceMetricEventsResponseItem` type
definition specifies the metric value as `row[TraceMetricKnownFieldKey.METRIC_VALUE]`.
This incorrect property access pattern is inconsistent with other parts of the codebase,
such as line 88 in the same file, and will result in the tooltip displaying `undefined`
instead of the actual numeric metric value.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
This sounds reasonable, we could either do row[TraceMetricKnownFieldKey.METRIC_VALUE] or alternatively row[field] to avoid hard coding things here.
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| if (result.data?.data) { | ||
| result.data.data = sortedResult; | ||
| } |
There was a problem hiding this comment.
Bug: Mutating Cached Query Data During Sorting Issue
The code mutates the cached query result by directly assigning to result.data.data. The sortedResult is computed using result.data?.data?.sort() which mutates the original array, and then it's assigned back to result.data.data. This violates React Query's immutability principle and can cause unexpected behavior with caching and stale data. The array should be copied before sorting (e.g., using [...result.data?.data].sort() or result.data?.data?.slice().sort()) to avoid mutating the cached response.
- Update useMetricOptions tests to match new API (no orderby, includes metric.unit field, client-side sorting) - Fix useMetricSamplesTable hook to properly pass enabled parameter to useProgressiveQuery - Remove disableExtrapolation option that was causing double API calls
| name: metricOptions[0]?.value ?? '', | ||
| type: metricOptions[0]?.type ?? '', | ||
| name: metricOptions[0].metricName, | ||
| type: metricOptions[0].metricType, |
There was a problem hiding this comment.
Bug: Metric Selection Resets on Options Update
The useEffect condition traceMetric.name !== metricOptions[0].metricName will override the user's metric selection whenever the first option in the list changes, even if the user has manually selected a different metric. The old code only set the metric when !traceMetric.name (i.e., when no metric was selected). This change can cause the metric selection to be unexpectedly reset when metricOptions updates, potentially creating an infinite loop if the user selects a non-first option and the options list updates.
| } | ||
| }); | ||
| } else { | ||
| query.statsPeriod = '1h'; // Default to a much smaller time window if not searching. |
There was a problem hiding this comment.
I'm inclined to say we should raise this to say 24 hours to capture metrics that are emitted hourly
| if (parts.length === 0) { | ||
| return ''; | ||
| } | ||
| if (parts.length === 1) { | ||
| return parts[0]; | ||
| } |
| attributes: row, | ||
| attributeTypes: meta.fields ?? {}, | ||
| highlightTerms: [], | ||
| logColors: getLogColors(SeverityLevel.INFO, theme), |
There was a problem hiding this comment.
Isn't this the metric's timestamp? I guess you're reusing the timestamp renderer from logs? Not necessary in this PR but it'd be good to pull this component into a shared one.
There was a problem hiding this comment.
We could pull it out, sure, but we should also make a fieldRenderers probably for metrics instead of these inline renders cells
| embedded={embedded} | ||
| > | ||
| {isValueColumn ? ( | ||
| <Tooltip showOnlyOnOverflow title={row.value}> |
There was a problem hiding this comment.
This sounds reasonable, we could either do row[TraceMetricKnownFieldKey.METRIC_VALUE] or alternatively row[field] to avoid hard coding things here.
| export function SamplesTab({traceMetric}: SamplesTabProps) { | ||
| return <MetricsSamplesTable traceMetric={traceMetric} />; | ||
| } |
There was a problem hiding this comment.
What is the purpose of this component? Just using the MetricsSamplesTable component is the same right?
There was a problem hiding this comment.
I was just keeping it for parity with aggregates for now, we -can- remove
| for (const key in ReadableQueryParams.prototype) { | ||
| delete target.query[key]; | ||
| } |
There was a problem hiding this comment.
Does this do anything? We encode the the query params into a json object and put it under location.query.metric.
| name: typedOption.value, | ||
| type: typedOption.type, | ||
| name: typedOption.metricName, | ||
| type: typedOption.metricType, |
There was a problem hiding this comment.
This change isn't fully handling the metric unit correctly. It fetches the unit for the metric and renders it in the option but doesn't propagate it anywhere. I'm almost inclined to say we should leave it for a separate PR because this gives the impression units should work when it doesn't.
There was a problem hiding this comment.
We could, but I'd rather leave it in and just update it in these spots since we'll have to shortly anyway
static/app/views/explore/metrics/metricsQueryParamsProvider.tsx
Outdated
Show resolved
Hide resolved
| const target: Location = {...location, query: {...location.query}}; | ||
| for (const key in ReadableQueryParams.prototype) { | ||
| delete target.query[key]; | ||
| } |
There was a problem hiding this comment.
Bug: Prototype loop fails to strip query parameters
The for (const key in ReadableQueryParams.prototype) loop attempts to iterate over prototype properties to delete query parameters. However, ReadableQueryParams is a class with instance properties defined in the constructor, not on the prototype. The prototype only contains the replace method. This means the loop will likely only delete the replace key (if it exists in the query object), not the actual query parameters like query, fields, sortBys, aggregateFields, etc. The function won't properly strip the metric-related query parameters as intended.
…102737) ### Summary Splits out tables and cleans up some code before infinite table + trace view changes.
…102737) ### Summary Splits out tables and cleans up some code before infinite table + trace view changes.
…102737) ### Summary Splits out tables and cleans up some code before infinite table + trace view changes.
Summary
Splits out tables and cleans up some code before infinite table + trace view changes.