Skip to content

Commit 361c184

Browse files
authored
chore(timeseries): Improve comparison logic (#101194)
A few more improvements to the comparison algorith. This loosens the comparison criteria somewhat, and improves some of the behaviour. - Improve `groupBy` type and docs - Improve `groupBy` parsing - Add support for logs - Ignore `dataScanned` in comparator - Apply loose comparison to all numeric keys
1 parent 1d154a0 commit 361c184

File tree

4 files changed

+28
-11
lines changed

4 files changed

+28
-11
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import {parseGroupBy} from './parseGroupBy';
22

33
describe('parseGroupBy', () => {
4-
it('returns undefined for "Other" group name', () => {
4+
it('returns null for "Other" group name', () => {
55
const result = parseGroupBy('Other', ['field1', 'field2']);
6-
expect(result).toBeUndefined();
6+
expect(result).toBeNull();
77
});
88

99
it('parses single field and value correctly', () => {
@@ -47,6 +47,6 @@ describe('parseGroupBy', () => {
4747

4848
it('handles empty fields array', () => {
4949
const result = parseGroupBy('', []);
50-
expect(result).toBeUndefined();
50+
expect(result).toBeNull();
5151
});
5252
});

static/app/utils/timeSeries/parseGroupBy.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import type {TimeSeriesGroupBy} from 'sentry/views/dashboards/widgets/common/typ
55
export function parseGroupBy(
66
groupName: string,
77
fields: string[]
8-
): TimeSeriesGroupBy[] | undefined {
8+
): TimeSeriesGroupBy[] | null {
99
if (groupName === 'Other') {
10-
return undefined;
10+
return null;
1111
}
1212

1313
const groupKeys = fields;
@@ -22,5 +22,5 @@ export function parseGroupBy(
2222
return groupBy.key || groupBy.value;
2323
});
2424

25-
return groupBys.length > 0 ? groupBys : undefined;
25+
return groupBys.length > 0 ? groupBys : null;
2626
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ export type TimeSeries = {
9090
* Represents the grouping information for the time series, if applicable.
9191
* e.g., if the initial request supplied a `groupBy` query param of `"span.op"`, the
9292
* `groupBy` of the `TimeSeries` could be `[{key: 'span.op': value: 'db' }]`
93-
* If the `excludeOther` query param is `true`, an "other" time series will be part of the response. `TimeSeries.meta.isOther` specifies the "other" time series.
93+
* If the `excludeOther` query param is `true`, an "other" time series will be part of the response. `TimeSeries.meta.isOther` specifies the "other" time series, and `groupBy` is `null` in that case
9494
*/
95-
groupBy?: TimeSeriesGroupBy[];
95+
groupBy?: TimeSeriesGroupBy[] | null;
9696
};
9797

9898
export type TabularValueType = AttributeValueType | null;

static/app/views/insights/common/queries/useSortedTimeSeries.tsx

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {decodeSorts} from 'sentry/utils/queryString';
2323
import {getTimeSeriesInterval} from 'sentry/utils/timeSeries/getTimeSeriesInterval';
2424
import {markDelayedData} from 'sentry/utils/timeSeries/markDelayedData';
2525
import {parseGroupBy} from 'sentry/utils/timeSeries/parseGroupBy';
26-
import {useFetchSpanTimeSeries} from 'sentry/utils/timeSeries/useFetchEventsTimeSeries';
26+
import {useFetchEventsTimeSeries} from 'sentry/utils/timeSeries/useFetchEventsTimeSeries';
2727
import type {MutableSearch} from 'sentry/utils/tokenizeSearch';
2828
import {useLocation} from 'sentry/utils/useLocation';
2929
import useOrganization from 'sentry/utils/useOrganization';
@@ -121,7 +121,8 @@ export const useSortedTimeSeries = <
121121
useIsSampled(0.1, key) &&
122122
organization.features.includes('explore-events-time-series-spot-check');
123123

124-
const timeSeriesResult = useFetchSpanTimeSeries(
124+
const timeSeriesResult = useFetchEventsTimeSeries(
125+
dataset ?? DiscoverDatasets.SPANS,
125126
{
126127
yAxis: yAxis as unknown as any,
127128
query: search,
@@ -360,16 +361,32 @@ export function convertEventsStatsToTimeSeriesData(
360361
return [delayedTimeSeries.meta.order ?? 0, delayedTimeSeries];
361362
}
362363

364+
const NUMERIC_KEYS: Array<symbol | string | number> = [
365+
'value',
366+
'sampleCount',
367+
'sampleRate',
368+
];
369+
363370
function comparator(
364371
valueA: unknown,
365372
valueB: unknown,
366373
key: symbol | string | number | undefined
367374
) {
368375
// Compare numbers by near equality, which makes the comparison less sensitive to small natural variations in value caused by request sequencing
369-
if (key === 'value' && typeof valueA === 'number' && typeof valueB === 'number') {
376+
if (
377+
key &&
378+
NUMERIC_KEYS.includes(key) &&
379+
typeof valueA === 'number' &&
380+
typeof valueB === 'number'
381+
) {
370382
return areNumbersAlmostEqual(valueA, valueB, 5);
371383
}
372384

385+
// This can be removed when ENG-5677 is resolved. There's a known bug here.
386+
if (key === 'dataScanned') {
387+
return true;
388+
}
389+
373390
// Otherwise use default deep comparison
374391
return undefined;
375392
}

0 commit comments

Comments
 (0)