Skip to content

Commit ae6ec7c

Browse files
authored
ref(explore): Improve conversion to TimeSeries (#100532)
Explore makes requests to `/events-stats/` but immediately converts that response to an array of `TimeSeries` objects, which are passed to charts and so on. The way it's done right now is a _slight_ hack, which I'm fixing. If the request is a "top events" grouped by environment, we end up with `TimeSeries` objects that look like this: ``` { yAxis: "prod", values: [... meta: { ... ``` This works fine because `TimeSeriesWidgetVisualization` parses the `yAxis` value and formats it correctly. A better approach is to actually _parse_ the response properly, and create a full `TimeSeries` object that looks like this: ``` { yAxis: "count(span.duration)", groupBy: [{key: "environment": value: "prod"}], meta: { isOther: false, ... ``` If we correctly populate `yAxis`, `groupBy`, and `isOther`, that allows `TimeSeriesVisualization` to correctly label everything by default. This removes the need for setting aliases, and improves compatibility. Incidentally, it also fixes DAIN-119! With a proper `groupBy`, we don't need to try and parse everything as a version, and cause false positives. Other than the fix for that small bug, this doesn't create any visible changes in the UI. This also helps me prepare for replacing `/events-stats/` with `/events-timeseries/` on this page. **NOTE:** This change creates a bit of duplication when parsing `groupBy`, but don't worry, it's temporary, since all this code will go away once we use `/events-timeseries/` here. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Refactors timeseries transformation to populate yAxis/groupBy/isOther and ordering, updates chart/formatter to use meta.isOther, adds parseGroupBy, and removes legacy isTimeSeriesOther. > > - **Explore timeseries transformation**: > - Refactor `transformToSeriesMap` and `convertEventsStatsToTimeSeriesData` to set `yAxis`, `groupBy` (via `parseGroupBy`), `meta.isOther`, and `meta.order`; support grouped multi-series; return series grouped by `yAxis` and sorted by `order`. > - **Formatting and visualization**: > - `formatTimeSeriesName` returns `"Other"` when `meta.isOther` and continues existing measurement/version/equation formatting. > - `chartVisualization` colors series using `s.meta.isOther` and removes alias-based legend formatting. > - **Types**: > - Export `TimeSeriesGroupBy` from `widgets/common/types`. > - **Utilities/Tests**: > - Add `parseGroupBy` with tests; remove `isTimeSeriesOther` and its tests; add tests for `Other` handling in formatter. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ea8e062. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent b660982 commit ae6ec7c

File tree

9 files changed

+174
-114
lines changed

9 files changed

+174
-114
lines changed

static/app/utils/timeSeries/isTimeSeriesOther.spec.tsx

Lines changed: 0 additions & 29 deletions
This file was deleted.

static/app/utils/timeSeries/isTimeSeriesOther.tsx

Lines changed: 0 additions & 8 deletions
This file was deleted.
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import {parseGroupBy} from './parseGroupBy';
2+
3+
describe('parseGroupBy', () => {
4+
it('returns undefined for "Other" group name', () => {
5+
const result = parseGroupBy('Other', ['field1', 'field2']);
6+
expect(result).toBeUndefined();
7+
});
8+
9+
it('parses single field and value correctly', () => {
10+
const result = parseGroupBy('value1', ['field1']);
11+
expect(result).toEqual([{key: 'field1', value: 'value1'}]);
12+
});
13+
14+
it('parses multiple fields and values correctly', () => {
15+
const result = parseGroupBy('value1,value2,value3', ['field1', 'field2', 'field3']);
16+
expect(result).toEqual([
17+
{key: 'field1', value: 'value1'},
18+
{key: 'field2', value: 'value2'},
19+
{key: 'field3', value: 'value3'},
20+
]);
21+
});
22+
23+
it('handles more values than fields by using empty strings for extra values', () => {
24+
const result = parseGroupBy('value1,value2', ['field1']);
25+
expect(result).toEqual([
26+
{key: 'field1', value: 'value1'},
27+
{key: '', value: 'value2'},
28+
]);
29+
});
30+
31+
it('handles more fields than values by using empty strings for missing values', () => {
32+
const result = parseGroupBy('value1', ['field1', 'field2']);
33+
expect(result).toEqual([
34+
{key: 'field1', value: 'value1'},
35+
{key: 'field2', value: ''},
36+
]);
37+
});
38+
39+
it('handles empty strings in values', () => {
40+
const result = parseGroupBy('value1,,value3', ['field1', 'field2', 'field3']);
41+
expect(result).toEqual([
42+
{key: 'field1', value: 'value1'},
43+
{key: 'field2', value: ''},
44+
{key: 'field3', value: 'value3'},
45+
]);
46+
});
47+
48+
it('handles empty fields array', () => {
49+
const result = parseGroupBy('', []);
50+
expect(result).toBeUndefined();
51+
});
52+
});
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import zipWith from 'lodash/zipWith';
2+
3+
import type {TimeSeriesGroupBy} from 'sentry/views/dashboards/widgets/common/types';
4+
5+
export function parseGroupBy(
6+
groupName: string,
7+
fields: string[]
8+
): TimeSeriesGroupBy[] | undefined {
9+
if (groupName === 'Other') {
10+
return undefined;
11+
}
12+
13+
const groupKeys = fields;
14+
const groupValues = groupName.split(',');
15+
16+
const groupBys = zipWith(groupKeys, groupValues, (key, value) => {
17+
return {
18+
key: key ?? '',
19+
value: value ?? '',
20+
};
21+
}).filter(groupBy => {
22+
return groupBy.key || groupBy.value;
23+
});
24+
25+
return groupBys.length > 0 ? groupBys : undefined;
26+
}

static/app/views/dashboards/widgets/common/types.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ export type TimeSeriesItem = {
6565
*/
6666
type IncompleteReason = 'INCOMPLETE_BUCKET';
6767

68-
type TimeSeriesGroupBy = {
68+
export type TimeSeriesGroupBy = {
6969
key: string;
7070
/**
7171
* The `value` of a `groupBy` can sometimes surprisingly be an array, because some datasets support array values. e.g., in the error dataset, the error type could be an array that looks like `["Exception", null, "TypeError"]`

static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesName.spec.tsx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,4 +148,16 @@ describe('formatSeriesName', () => {
148148
expect(formatTimeSeriesName(timeSeries)).toEqual(result);
149149
});
150150
});
151+
152+
describe('other', () => {
153+
it('Formats "Other"', () => {
154+
const timeSeries = TimeSeriesFixture();
155+
timeSeries.meta = {
156+
...timeSeries.meta,
157+
isOther: true,
158+
};
159+
160+
expect(formatTimeSeriesName(timeSeries)).toBe('Other');
161+
});
162+
});
151163
});

static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesName.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import {t} from 'sentry/locale';
12
import {
23
getAggregateArg,
34
getMeasurementSlug,
@@ -12,6 +13,10 @@ export function formatTimeSeriesName(timeSeries: TimeSeries): string {
1213
// If the timeSeries has `groupBy` information, the label is made by
1314
// concatenating the values of the groupBy, since there's no point repeating
1415
// the name of the Y axis multiple times in the legend.
16+
if (timeSeries.meta.isOther) {
17+
return t('Other');
18+
}
19+
1520
if (timeSeries.groupBy?.length && timeSeries.groupBy.length > 0) {
1621
return `${timeSeries.groupBy
1722
?.map(groupBy => {

static/app/views/explore/components/chart/chartVisualization.tsx

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import styled from '@emotion/styled';
66
import TransparentLoadingMask from 'sentry/components/charts/transparentLoadingMask';
77
import {t} from 'sentry/locale';
88
import type {ReactEchartsRef} from 'sentry/types/echarts';
9-
import {isTimeSeriesOther} from 'sentry/utils/timeSeries/isTimeSeriesOther';
109
import {markDelayedData} from 'sentry/utils/timeSeries/markDelayedData';
1110
import usePrevious from 'sentry/utils/usePrevious';
1211
import {Area} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/area';
@@ -17,7 +16,6 @@ import {TimeSeriesWidgetVisualization} from 'sentry/views/dashboards/widgets/tim
1716
import {Widget} from 'sentry/views/dashboards/widgets/widget/widget';
1817
import type {ChartInfo} from 'sentry/views/explore/components/chart/types';
1918
import {SAMPLING_MODE} from 'sentry/views/explore/hooks/useProgressiveQuery';
20-
import {prettifyAggregation} from 'sentry/views/explore/utils';
2119
import {ChartType} from 'sentry/views/insights/common/components/chart';
2220
import {INGESTION_DELAY} from 'sentry/views/insights/settings';
2321

@@ -38,8 +36,6 @@ export function ChartVisualization({
3836
const theme = useTheme();
3937

4038
const plottables = useMemo(() => {
41-
const formattedYAxis = prettifyAggregation(chartInfo.yAxis) ?? chartInfo.yAxis;
42-
4339
const DataPlottableConstructor =
4440
chartInfo.chartType === ChartType.LINE
4541
? Line
@@ -55,13 +51,12 @@ export function ChartVisualization({
5551
// values instead of the aggregate function.
5652
if (s.yAxis === chartInfo.yAxis) {
5753
return new DataPlottableConstructor(markDelayedData(s, INGESTION_DELAY), {
58-
alias: formattedYAxis ?? chartInfo.yAxis,
59-
color: isTimeSeriesOther(s) ? theme.chartOther : undefined,
54+
color: s.meta.isOther ? theme.chartOther : undefined,
6055
stack: 'all',
6156
});
6257
}
6358
return new DataPlottableConstructor(markDelayedData(s, INGESTION_DELAY), {
64-
color: isTimeSeriesOther(s) ? theme.chartOther : undefined,
59+
color: s.meta.isOther ? theme.chartOther : undefined,
6560
stack: 'all',
6661
});
6762
});

0 commit comments

Comments
 (0)