Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 0 additions & 29 deletions static/app/utils/timeSeries/isTimeSeriesOther.spec.tsx

This file was deleted.

8 changes: 0 additions & 8 deletions static/app/utils/timeSeries/isTimeSeriesOther.tsx

This file was deleted.

52 changes: 52 additions & 0 deletions static/app/utils/timeSeries/parseGroupBy.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import {parseGroupBy} from './parseGroupBy';

describe('parseGroupBy', () => {
it('returns undefined for "Other" group name', () => {
const result = parseGroupBy('Other', ['field1', 'field2']);
expect(result).toBeUndefined();
});

it('parses single field and value correctly', () => {
const result = parseGroupBy('value1', ['field1']);
expect(result).toEqual([{key: 'field1', value: 'value1'}]);
});

it('parses multiple fields and values correctly', () => {
const result = parseGroupBy('value1,value2,value3', ['field1', 'field2', 'field3']);
expect(result).toEqual([
{key: 'field1', value: 'value1'},
{key: 'field2', value: 'value2'},
{key: 'field3', value: 'value3'},
]);
});

it('handles more values than fields by using empty strings for extra values', () => {
const result = parseGroupBy('value1,value2', ['field1']);
expect(result).toEqual([
{key: 'field1', value: 'value1'},
{key: '', value: 'value2'},
]);
});

it('handles more fields than values by using empty strings for missing values', () => {
const result = parseGroupBy('value1', ['field1', 'field2']);
expect(result).toEqual([
{key: 'field1', value: 'value1'},
{key: 'field2', value: ''},
]);
});

it('handles empty strings in values', () => {
const result = parseGroupBy('value1,,value3', ['field1', 'field2', 'field3']);
expect(result).toEqual([
{key: 'field1', value: 'value1'},
{key: 'field2', value: ''},
{key: 'field3', value: 'value3'},
]);
});

it('handles empty fields array', () => {
const result = parseGroupBy('', []);
expect(result).toBeUndefined();
});
});
26 changes: 26 additions & 0 deletions static/app/utils/timeSeries/parseGroupBy.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import zipWith from 'lodash/zipWith';

import type {TimeSeriesGroupBy} from 'sentry/views/dashboards/widgets/common/types';

export function parseGroupBy(
groupName: string,
fields: string[]
): TimeSeriesGroupBy[] | undefined {
if (groupName === 'Other') {
return undefined;
}

const groupKeys = fields;
const groupValues = groupName.split(',');
Copy link
Member

Choose a reason for hiding this comment

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

This makes the assumption that each group value won't have any commas.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 it does make that assumption, yes. I don't think there's actually any way for me to figure out which commas are the delimiters, even knowing how many fields there were originally ... but this problem will go away when we use /events-timeseries/ which separates each field completely

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up making this more permissive, and creating empty keys/values when necessary. The values end up getting concatenated with commas later anyway and the keys are not readily visible to anyone, so we should be good. Good catch, thanks!


const groupBys = zipWith(groupKeys, groupValues, (key, value) => {
return {
key: key ?? '',
value: value ?? '',
};
}).filter(groupBy => {
return groupBy.key || groupBy.value;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: parseGroupBy Truncates Extra Values Instead of Pairing

The parseGroupBy function's use of zipWith drops extra values from groupName when there are more values than fields. This contradicts test expectations that these additional values should be included, paired with empty keys.

Fix in Cursor Fix in Web


return groupBys.length > 0 ? groupBys : undefined;
}
2 changes: 1 addition & 1 deletion static/app/views/dashboards/widgets/common/types.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export type TimeSeriesItem = {
*/
type IncompleteReason = 'INCOMPLETE_BUCKET';

type TimeSeriesGroupBy = {
export type TimeSeriesGroupBy = {
key: string;
/**
* 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"]`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,16 @@ describe('formatSeriesName', () => {
expect(formatTimeSeriesName(timeSeries)).toEqual(result);
});
});

describe('other', () => {
it('Formats "Other"', () => {
const timeSeries = TimeSeriesFixture();
timeSeries.meta = {
...timeSeries.meta,
isOther: true,
};

expect(formatTimeSeriesName(timeSeries)).toBe('Other');
});
});
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {t} from 'sentry/locale';
import {
getAggregateArg,
getMeasurementSlug,
Expand All @@ -12,6 +13,10 @@ export function formatTimeSeriesName(timeSeries: TimeSeries): string {
// If the timeSeries has `groupBy` information, the label is made by
// concatenating the values of the groupBy, since there's no point repeating
// the name of the Y axis multiple times in the legend.
if (timeSeries.meta.isOther) {
return t('Other');
}

if (timeSeries.groupBy?.length && timeSeries.groupBy.length > 0) {
return `${timeSeries.groupBy
?.map(groupBy => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import styled from '@emotion/styled';
import TransparentLoadingMask from 'sentry/components/charts/transparentLoadingMask';
import {t} from 'sentry/locale';
import type {ReactEchartsRef} from 'sentry/types/echarts';
import {isTimeSeriesOther} from 'sentry/utils/timeSeries/isTimeSeriesOther';
import {markDelayedData} from 'sentry/utils/timeSeries/markDelayedData';
import usePrevious from 'sentry/utils/usePrevious';
import {Area} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/area';
Expand All @@ -17,7 +16,6 @@ import {TimeSeriesWidgetVisualization} from 'sentry/views/dashboards/widgets/tim
import {Widget} from 'sentry/views/dashboards/widgets/widget/widget';
import type {ChartInfo} from 'sentry/views/explore/components/chart/types';
import {SAMPLING_MODE} from 'sentry/views/explore/hooks/useProgressiveQuery';
import {prettifyAggregation} from 'sentry/views/explore/utils';
import {ChartType} from 'sentry/views/insights/common/components/chart';
import {INGESTION_DELAY} from 'sentry/views/insights/settings';

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

const plottables = useMemo(() => {
const formattedYAxis = prettifyAggregation(chartInfo.yAxis) ?? chartInfo.yAxis;

const DataPlottableConstructor =
chartInfo.chartType === ChartType.LINE
? Line
Expand All @@ -55,13 +51,12 @@ export function ChartVisualization({
// values instead of the aggregate function.
if (s.yAxis === chartInfo.yAxis) {
return new DataPlottableConstructor(markDelayedData(s, INGESTION_DELAY), {
alias: formattedYAxis ?? chartInfo.yAxis,
color: isTimeSeriesOther(s) ? theme.chartOther : undefined,
color: s.meta.isOther ? theme.chartOther : undefined,
stack: 'all',
});
}
return new DataPlottableConstructor(markDelayedData(s, INGESTION_DELAY), {
color: isTimeSeriesOther(s) ? theme.chartOther : undefined,
color: s.meta.isOther ? theme.chartOther : undefined,
stack: 'all',
});
});
Expand Down
Loading
Loading