Skip to content

fix(tracemetrics): Parse units off response in events request for alerts #109608

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.

Sign up for GitHub

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

Merged
narsaynorath merged 5 commits into from
Mar 4, 2026
Merged

fix(tracemetrics): Parse units off response in events request for alerts #109608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a855152
fix(tracemetrics): Parse units off response in events request
narsaynorath Feb 27, 2026
f5b9424
fix double converting of size
narsaynorath Mar 3, 2026
3918de2
Merge branch 'master' into nar/fix/tracemetrics-parse-units-off-respo…
narsaynorath Mar 3, 2026
86f174c
Merge branch 'master' into nar/fix/tracemetrics-parse-units-off-respo…
narsaynorath Mar 4, 2026
a1063fa
add raw key lookup for unit
narsaynorath Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 117 additions & 0 deletions static/app/components/charts/eventsRequest.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,123 @@ describe('EventsRequest', () => {
);
});

it('passes timeseriesResultsUnits to child for single yAxis', async () => {
(doEventsRequest as jest.Mock).mockImplementation(() =>
Promise.resolve({
data: [[new Date(), [COUNT_OBJ]]],
start: 1627402280,
end: 1627402398,
meta: {
fields: {
p95_measurements_custom: 'size',
},
units: {
p95_measurements_custom: 'kibibyte',
},
},
})
);
render(
<EventsRequest {...DEFAULTS} yAxis="p95(measurements.custom)">
{mock}
</EventsRequest>
);

await waitFor(() =>
expect(mock).toHaveBeenLastCalledWith(
expect.objectContaining({
timeseriesResultsTypes: {'p95(measurements.custom)': 'size'},
timeseriesResultsUnits: {'p95(measurements.custom)': 'kibibyte'},
})
)
);
});

it('passes timeseriesResultsUnits to child for multiple yAxis', async () => {
(doEventsRequest as jest.Mock).mockImplementation(() =>
Promise.resolve({
'p95(measurements.custom)': {
data: [[new Date(), [COUNT_OBJ]]],
start: 1627402280,
end: 1627402398,
meta: {
fields: {
p95_measurements_custom: 'size',
},
units: {
p95_measurements_custom: 'kibibyte',
},
},
},
'p50(measurements.lcp)': {
data: [[new Date(), [COUNT_OBJ]]],
start: 1627402280,
end: 1627402398,
meta: {
fields: {
p50_measurements_lcp: 'duration',
},
units: {
p50_measurements_lcp: 'millisecond',
},
},
},
})
);
render(
<EventsRequest
{...DEFAULTS}
yAxis={['p95(measurements.custom)', 'p50(measurements.lcp)']}
>
{mock}
</EventsRequest>
);

await waitFor(() =>
expect(mock).toHaveBeenLastCalledWith(
expect.objectContaining({
timeseriesResultsTypes: {
'p95(measurements.custom)': 'size',
'p50(measurements.lcp)': 'duration',
},
timeseriesResultsUnits: {
'p95(measurements.custom)': 'kibibyte',
'p50(measurements.lcp)': 'millisecond',
},
})
)
);
});

it('does not include timeseriesResultsUnits when meta has no units', async () => {
(doEventsRequest as jest.Mock).mockImplementation(() =>
Promise.resolve({
data: [[new Date(), [COUNT_OBJ]]],
start: 1627402280,
end: 1627402398,
meta: {
fields: {
'count()': 'integer',
},
},
})
);
render(
<EventsRequest {...DEFAULTS} yAxis="count()">
{mock}
</EventsRequest>
);

await waitFor(() =>
expect(mock).toHaveBeenLastCalledWith(
expect.objectContaining({
timeseriesResultsTypes: {'count()': 'integer'},
timeseriesResultsUnits: undefined,
})
)
);
});

it('scales timeseries values according to unit meta', async () => {
(doEventsRequest as jest.Mock).mockImplementation(() =>
Promise.resolve({
Expand Down
29 changes: 25 additions & 4 deletions static/app/components/charts/eventsRequest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import type {
} from 'sentry/types/organization';
import {defined} from 'sentry/utils';
import {DURATION_UNITS, SIZE_UNITS} from 'sentry/utils/discover/fieldRenderers';
import type {AggregationOutputType} from 'sentry/utils/discover/fields';
import type {AggregationOutputType, DataUnit} from 'sentry/utils/discover/fields';
import {getAggregateAlias, stripEquationPrefix} from 'sentry/utils/discover/fields';
import type {DiscoverDatasets} from 'sentry/utils/discover/types';
import type {SamplingMode} from 'sentry/views/explore/hooks/useProgressiveQuery';
Expand All @@ -38,6 +38,7 @@ type TimeSeriesData = {
// timeseries data
timeseriesData?: Series[];
timeseriesResultsTypes?: Record<string, AggregationOutputType>;
timeseriesResultsUnits?: Record<string, DataUnit>;
timeseriesTotals?: {count: number};
yAxis?: string | string[];
};
Expand Down Expand Up @@ -548,11 +549,19 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
)
.sort((a, b) => a[0] - b[0]);
const timeseriesResultsTypes: Record<string, AggregationOutputType> = {};
const timeseriesResultsUnits: Record<string, DataUnit> = {};
Object.keys(timeseriesData).forEach(key => {
const fieldsMeta = timeseriesData[key]!.meta?.fields[getAggregateAlias(key)];
const alias = getAggregateAlias(key);
const fieldsMeta =
timeseriesData[key]!.meta?.fields?.[key] ??
timeseriesData[key]!.meta?.fields?.[alias];
if (fieldsMeta) {
timeseriesResultsTypes[key] = fieldsMeta;
}
const unitsMeta = timeseriesData[key]!.meta?.units?.[alias];
if (unitsMeta) {
timeseriesResultsUnits[key] = unitsMeta as DataUnit;
}
});
const results: Series[] = sortedTimeseriesData.map(item => {
return item[1];
Expand All @@ -573,17 +582,27 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
previousTimeseriesData,
seriesAdditionalInfo,
timeseriesResultsTypes,
timeseriesResultsUnits,
// sometimes we want to reference props that were given to EventsRequest
...props,
});
}
if (timeseriesData) {
const yAxisKey = yAxis && (typeof yAxis === 'string' ? yAxis : yAxis[0]);
const yAxisFieldType =
yAxisKey && timeseriesData.meta?.fields[getAggregateAlias(yAxisKey)];
yAxisKey &&
(timeseriesData.meta?.fields[getAggregateAlias(yAxisKey)] ||
timeseriesData.meta?.fields[yAxisKey]);
const timeseriesResultsTypes = yAxisFieldType
? {[yAxisKey]: yAxisFieldType}
: undefined;
const yAxisAlias = yAxisKey && getAggregateAlias(yAxisKey);
const yAxisUnit =
yAxisAlias &&
(timeseriesData.meta?.units?.[yAxisAlias] ??
timeseriesData.meta?.units?.[yAxisKey]);
Comment on lines +602 to +603
Copy link
Member Author

@narsaynorath narsaynorath Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to check for both here because the backend no longer requires us to send the aggregate alias, so this would allow both the alias and the raw series name to match.

const timeseriesResultsUnits =
yAxisKey && yAxisUnit ? {[yAxisKey]: yAxisUnit as DataUnit} : undefined;
const {
data: transformedTimeseriesData,
comparisonData: transformedComparisonTimeseriesData,
Expand Down Expand Up @@ -625,6 +644,7 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
timeAggregatedData,
timeframe,
timeseriesResultsTypes,
timeseriesResultsUnits,
// sometimes we want to reference props that were given to EventsRequest
...props,
});
Expand All @@ -650,7 +670,8 @@ export function transformTimeseriesData(
): Series[] {
let scale = 1;
if (seriesName) {
const unit = meta?.units?.[getAggregateAlias(seriesName)];
const unit =
meta?.units?.[seriesName] ?? meta?.units?.[getAggregateAlias(seriesName)];
// Scale series values to milliseconds or bytes depending on units from meta
scale =
((unit &&
Expand Down
11 changes: 11 additions & 0 deletions static/app/views/alerts/rules/metric/triggers/chart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import type {
Organization,
} from 'sentry/types/organization';
import type {Project} from 'sentry/types/project';
import type {AggregationOutputType, DataUnit} from 'sentry/utils/discover/fields';
import {DiscoverDatasets} from 'sentry/utils/discover/types';
import {parsePeriodToHours} from 'sentry/utils/duration/parsePeriodToHours';
import {shouldShowOnDemandMetricAlertUI} from 'sentry/utils/onDemandMetrics/features';
Expand Down Expand Up @@ -323,6 +324,8 @@ class TriggersChart extends PureComponent<Props, State> {
errored,
orgFeatures,
seriesAdditionalInfo,
timeseriesResultsTypes,
timeseriesResultsUnits,
}: {
isLoading: boolean;
isQueryValid: boolean;
Expand All @@ -335,6 +338,8 @@ class TriggersChart extends PureComponent<Props, State> {
errored?: boolean;
minutesThresholdToDisplaySeconds?: number;
seriesAdditionalInfo?: Record<string, any>;
timeseriesResultsTypes?: Record<string, AggregationOutputType>;
timeseriesResultsUnits?: Record<string, DataUnit>;
}) {
const {
triggers,
Expand Down Expand Up @@ -401,6 +406,8 @@ class TriggersChart extends PureComponent<Props, State> {
aggregate={aggregate}
minutesThresholdToDisplaySeconds={minutesThresholdToDisplaySeconds}
isExtrapolatedData={showExtrapolatedChartData}
timeseriesResultsTypes={timeseriesResultsTypes}
timeseriesResultsUnits={timeseriesResultsUnits}
/>
)}

Expand Down Expand Up @@ -700,6 +707,8 @@ class TriggersChart extends PureComponent<Props, State> {
reloading,
timeseriesData,
comparisonTimeseriesData,
timeseriesResultsTypes,
timeseriesResultsUnits,
}) => {
let comparisonMarkLines: LineChartSeries[] = [];
if (renderComparisonStats && comparisonTimeseriesData) {
Expand All @@ -723,6 +732,8 @@ class TriggersChart extends PureComponent<Props, State> {
isQueryValid,
errored,
orgFeatures: organization.features,
timeseriesResultsTypes,
timeseriesResultsUnits,
});
}}
</EventsRequest>
Expand Down
53 changes: 48 additions & 5 deletions static/app/views/alerts/rules/metric/triggers/chart/thresholdsChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,22 @@ import {DEFAULT_STATS_PERIOD} from 'sentry/constants';
import {space} from 'sentry/styles/space';
import type {PageFilters} from 'sentry/types/core';
import type {Series} from 'sentry/types/echarts';
import {defined} from 'sentry/utils';
import {
axisLabelFormatterUsingAggregateOutputType,
getDurationUnit,
tooltipFormatter,
} from 'sentry/utils/discover/charts';
import type {AggregationOutputType, DataUnit} from 'sentry/utils/discover/fields';
import {aggregateOutputType, RateUnit} from 'sentry/utils/discover/fields';
import type {MetricRule, Trigger} from 'sentry/views/alerts/rules/metric/types';
import {
AlertRuleThresholdType,
AlertRuleTriggerType,
} from 'sentry/views/alerts/rules/metric/types';
import {getAnomalyMarkerSeries} from 'sentry/views/alerts/rules/metric/utils/anomalyChart';
import type {Anomaly} from 'sentry/views/alerts/types';
import {alertAxisFormatter, alertTooltipValueFormatter} from 'sentry/views/alerts/utils';
import {alertAxisFormatter, isSessionAggregate} from 'sentry/views/alerts/utils';
import {getChangeStatus} from 'sentry/views/alerts/utils/getChangeStatus';

type DefaultProps = {
Expand All @@ -45,6 +53,8 @@ type Props = DefaultProps & {
maxValue?: number;
minValue?: number;
minutesThresholdToDisplaySeconds?: number;
timeseriesResultsTypes?: Record<string, AggregationOutputType>;
timeseriesResultsUnits?: Record<string, DataUnit>;
} & Partial<PageFilters['datetime']>;

const CHART_GRID = {
Expand Down Expand Up @@ -206,6 +216,8 @@ export default class ThresholdsChart extends PureComponent<Props> {
resolveThreshold,
anomalies = [],
theme,
timeseriesResultsTypes,
timeseriesResultsUnits,
} = this.props;

const dataWithoutRecentBucket = data?.map(({data: eventData, ...restOfData}) => {
Expand All @@ -230,12 +242,33 @@ export default class ThresholdsChart extends PureComponent<Props> {
})
);

// Resolve output type and unit from server-provided metadata, falling back
// to inferring from the aggregate string when not available.
const outputType =
timeseriesResultsTypes?.[aggregate] ?? aggregateOutputType(aggregate);
const isDurationChart = outputType === 'duration';
const durationUnit = isDurationChart
? getDurationUnit(dataWithoutRecentBucket)
: undefined;
const rateUnit = Object.values(RateUnit).includes(
timeseriesResultsUnits?.[aggregate] as RateUnit
)
? (timeseriesResultsUnits?.[aggregate] as RateUnit)
: undefined;

const chartOptions = {
tooltip: {
// use the main aggregate for all series (main, min, max, avg, comparison)
// to format all values similarly
valueFormatter: (value: number) =>
alertTooltipValueFormatter(value, aggregate, aggregate),
valueFormatter: (value: number) => {
if (isSessionAggregate(aggregate)) {
return defined(value) ? `${value}%` : '\u2015';
}
const type =
timeseriesResultsTypes?.[aggregate] ?? aggregateOutputType(aggregate);
const unit = timeseriesResultsUnits?.[aggregate];
return tooltipFormatter(value, type, unit);
},

formatAxisLabel: (
value: number,
Expand Down Expand Up @@ -305,8 +338,18 @@ export default class ThresholdsChart extends PureComponent<Props> {
resolveThreshold === '' ? null : resolveThreshold
),
axisLabel: {
formatter: (value: number) =>
alertAxisFormatter(value, data[0]!.seriesName, aggregate),
formatter: (value: number) => {
if (timeseriesResultsTypes && !isSessionAggregate(aggregate)) {
return axisLabelFormatterUsingAggregateOutputType(
value,
outputType,
true,
durationUnit,
rateUnit
);
Copy link
Contributor

@cursor cursor bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Axis label formatter missing sizeUnit parameter for size metrics

Medium Severity

The call to axisLabelFormatterUsingAggregateOutputType passes durationUnit and rateUnit but omits the sizeUnit parameter (7th argument). For size-type aggregates like p95(measurements.custom) with unit kibibyte, the formatter defaults to SizeUnit.BYTE, producing incorrect axis labels. The unit info is available in timeseriesResultsUnits but isn't extracted and forwarded like rateUnit and durationUnit are.

Fix in Cursor Fix in Web

Copy link
Member Author

@narsaynorath narsaynorath Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay. I agree this is weird but in the existing code we already normalized to bytes, so passing in the unit here would have doubled the normalization. I suppose we could get rid of the normalization but I wanted to avoid touching any of the existing behaviour to ensure this only applied for when unit was supplied which is only applicable to tracemetricnar/fix/tracemetrics-parse-units-off-response-in-events-request. Removing the normalization mean ensuring all of the existing code paths for other parts of the app reaching here needed to be checked properly too.

}
return alertAxisFormatter(value, data[0]!.seriesName, aggregate);
},
},
splitLine: {
show: false,
Expand Down
Loading