feat(metrics): Add metrics section to issue details #103092
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
static/app/views/explore/metrics/metricInfoTabs/metricsSamplesTableRow.tsx
Show resolved
Hide resolved
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| traceIds={freeze?.traceIds ?? []} | ||
| tracePeriod={freeze?.tracePeriod} | ||
| > | ||
| <QueryParamsContextProvider | ||
| <QueryContextProvider | ||
| queryParams={queryParams} | ||
| setQueryParams={setWritableQueryParams} | ||
| isUsingDefaultFields | ||
| shouldManageFields={false} | ||
| > | ||
| {children} | ||
| </QueryParamsContextProvider> | ||
| </QueryContextProvider> |
There was a problem hiding this comment.
Bug: QueryContextProvider is incorrectly instantiated with MetricsStateQueryParamsProvider due to incompatible prop signatures, causing a TypeScript compilation error.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The QueryContextProvider is incorrectly used with MetricsStateQueryParamsProvider when isStateBased=true. This leads to a TypeScript compilation error because MetricsStateQueryParamsProvider does not accept queryParams, setQueryParams, isUsingDefaultFields, and shouldManageFields props, which are expected by QueryContextProvider. This fundamental type mismatch prevents the code from compiling.
💡 Suggested Fix
Either MetricsStateQueryParamsProvider needs to accept frozenParams to receive parent state, or the caller must stop passing incompatible props like queryParams and setQueryParams when isStateBased is true.
🤖 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/metricsQueryParams.tsx#L82-L99
Potential issue: The `QueryContextProvider` is incorrectly used with
`MetricsStateQueryParamsProvider` when `isStateBased=true`. This leads to a TypeScript
compilation error because `MetricsStateQueryParamsProvider` does not accept
`queryParams`, `setQueryParams`, `isUsingDefaultFields`, and `shouldManageFields` props,
which are expected by `QueryContextProvider`. This fundamental type mismatch prevents
the code from compiling.
Did we get this right? 👍 / 👎 to inform future reviews.
| export function defaultSortBys(fields: string[]): Sort[] { | ||
| // TODO: Handle compound sort-by. | ||
| return [{field: fields[0]!, kind: 'desc'}]; |
There was a problem hiding this comment.
Bug: defaultSortBys in metrics module sorts by id instead of prioritizing timestamp, deviating from established codebase patterns.
Severity: HIGH | Confidence: 0.90
🔍 Detailed Analysis
The defaultSortBys function in the new metrics module sorts by fields[0] (which is 'id' when called with ['id', 'timestamp']), instead of prioritizing 'timestamp'. This deviates from the established pattern in logsQueryParams.tsx, spansQueryParams.tsx, and the shared sortBys.tsx, all of which prioritize 'timestamp' for sorting. Consequently, metrics samples will sort by 'id' descending, altering user experience and violating a standard UX pattern.
💡 Suggested Fix
Modify the defaultSortBys function in the metrics module to prioritize 'timestamp' when available, consistent with other modules and shared utilities.
🤖 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/metricQuery.tsx#L135-L137
Potential issue: The `defaultSortBys` function in the new metrics module sorts by
`fields[0]` (which is 'id' when called with `['id', 'timestamp']`), instead of
prioritizing 'timestamp'. This deviates from the established pattern in
`logsQueryParams.tsx`, `spansQueryParams.tsx`, and the shared `sortBys.tsx`, all of
which prioritize 'timestamp' for sorting. Consequently, metrics samples will sort by
'id' descending, altering user experience and violating a standard UX pattern.
Did we get this right? 👍 / 👎 to inform future reviews.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| disabled: embedded | ||
| ? false | ||
| : !traceMetric?.name || isMetricOptionsEmpty || !!overrideTableData, | ||
| limit: embedded ? EMBEDDED_RESULT_LIMIT : RESULT_LIMIT, |
There was a problem hiding this comment.
Bug: Optimize API: Disable Unused Queries
When embedded=true and overrideTableData is provided, the query is not disabled but the fetched data is never used since overrideTableData takes precedence in rendering. This causes unnecessary API requests. The disabled condition should account for overrideTableData even when embedded.
| > | ||
| <Flex direction="column" gap="xl"> | ||
| <MetricsSamplesTable embedded overrideTableData={abbreviatedTableData} /> | ||
| {result.data && result.data.length > 5 ? ( |
There was a problem hiding this comment.
Can we put this 5 into a const somewhere?
| setQueryParams={() => {}} | ||
| traceMetric={{name: '', type: ''}} | ||
| setTraceMetric={() => {}} | ||
| removeMetric={() => {}} |
There was a problem hiding this comment.
The signature is a little odd here as it forces you to pass these empty values but this is fine for now I guess.
| if (!feature) { | ||
| return null; | ||
| } | ||
|
|
||
| if (!traceId) { | ||
| // If there isn't a traceId, we shouldn't show metrics since they are trace specific | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Should these be moved to the wrapper component?
Adds a metric sections to issue details, only shown when there are >0 metrics and there is a trace.
Also in this PR; modifying the providers to allow state based for it being embedded and searchable inside issue details with spamming url params.
Closes LOGS-369