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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {TimeSeriesFixture} from 'sentry-fixture/timeSeries';

import {formatTimeSeriesName} from './formatTimeSeriesName';
import {formatTimeSeriesLabel} from './formatTimeSeriesLabel';

describe('formatSeriesName', () => {
describe('releases', () => {
Expand All @@ -12,7 +12,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -26,7 +26,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -40,7 +40,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -53,7 +53,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -66,7 +66,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -79,7 +79,7 @@ describe('formatSeriesName', () => {
yAxis: name,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand Down Expand Up @@ -145,7 +145,7 @@ describe('formatSeriesName', () => {
groupBy,
});

expect(formatTimeSeriesName(timeSeries)).toEqual(result);
expect(formatTimeSeriesLabel(timeSeries)).toEqual(result);
});
});

Expand All @@ -157,7 +157,7 @@ describe('formatSeriesName', () => {
isOther: true,
};

expect(formatTimeSeriesName(timeSeries)).toBe('Other');
expect(formatTimeSeriesLabel(timeSeries)).toBe('Other');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import {t} from 'sentry/locale';
import {
getAggregateArg,
getMeasurementSlug,
maybeEquationAlias,
stripEquationPrefix,
} from 'sentry/utils/discover/fields';
import {formatVersion} from 'sentry/utils/versions/formatVersion';
import WidgetLegendNameEncoderDecoder from 'sentry/views/dashboards/widgetLegendNameEncoderDecoder';
import type {TimeSeries} from 'sentry/views/dashboards/widgets/common/types';

export function formatTimeSeriesLabel(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 => {
if (Array.isArray(groupBy.value)) {
return JSON.stringify(groupBy.value);
}

if (groupBy.key === 'release') {
return formatVersion(groupBy.value);
}

return groupBy.value;
})
.join(',')}`;
}

let {yAxis: seriesName} = timeSeries;

// Decode from series name disambiguation
seriesName = WidgetLegendNameEncoderDecoder.decodeSeriesNameForLegend(seriesName)!;

// Attempt to parse the `seriesName` as a version. A correct `TimeSeries`
// would have a `yAxis` like `p50(span.duration)` with a `groupBy` like
// `[{key: "release", value: "proj@1.2.3"}]`. `groupBy` was only introduced
// recently though, so many `TimeSeries` instead mash the group by information
// into the `yAxis` property, e.g., the `yAxis` might have been set to
// `"proj@1.2.3"` just to get the correct rendering in the chart legend. We
// cover these cases by parsing the `yAxis` as a series name. This works badly
// because sometimes it'll interpet email addresses as versions, which causes
// bugs. We should update all usages of `TimeSeriesWidgetVisualization` to
// correctly specify `yAxis` and `groupBy`, and/or to use the time
// `/events-timeseries` endpoint which does this automatically.
seriesName = formatVersion(seriesName);

// Check for special-case measurement formatting
const arg = getAggregateArg(seriesName);
if (arg) {
const slug = getMeasurementSlug(arg);

if (slug) {
seriesName = slug.toUpperCase();
}
}

// Strip equation prefix
if (maybeEquationAlias(seriesName)) {
seriesName = stripEquationPrefix(seriesName);
}

return seriesName;
}
Original file line number Diff line number Diff line change
@@ -1,70 +1,15 @@
import {t} from 'sentry/locale';
import {
getAggregateArg,
getMeasurementSlug,
maybeEquationAlias,
stripEquationPrefix,
} from 'sentry/utils/discover/fields';
import {formatVersion} from 'sentry/utils/versions/formatVersion';
import WidgetLegendNameEncoderDecoder from 'sentry/views/dashboards/widgetLegendNameEncoderDecoder';
import type {TimeSeries} from 'sentry/views/dashboards/widgets/common/types';

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');
}
let name = `${timeSeries.yAxis}`;

if (timeSeries.groupBy?.length && timeSeries.groupBy.length > 0) {
return `${timeSeries.groupBy
if (timeSeries.groupBy?.length) {
name += ` : ${timeSeries.groupBy
?.map(groupBy => {
if (Array.isArray(groupBy.value)) {
return JSON.stringify(groupBy.value);
}

if (groupBy.key === 'release') {
return formatVersion(groupBy.value);
}

return groupBy.value;
return `${groupBy.key} : ${groupBy.value}`;
})
.join(',')}`;
}

let {yAxis: seriesName} = timeSeries;

// Decode from series name disambiguation
seriesName = WidgetLegendNameEncoderDecoder.decodeSeriesNameForLegend(seriesName)!;

// Attempt to parse the `seriesName` as a version. A correct `TimeSeries`
// would have a `yAxis` like `p50(span.duration)` with a `groupBy` like
// `[{key: "release", value: "proj@1.2.3"}]`. `groupBy` was only introduced
// recently though, so many `TimeSeries` instead mash the group by information
// into the `yAxis` property, e.g., the `yAxis` might have been set to
// `"proj@1.2.3"` just to get the correct rendering in the chart legend. We
// cover these cases by parsing the `yAxis` as a series name. This works badly
// because sometimes it'll interpet email addresses as versions, which causes
// bugs. We should update all usages of `TimeSeriesWidgetVisualization` to
// correctly specify `yAxis` and `groupBy`, and/or to use the time
// `/events-timeseries` endpoint which does this automatically.
seriesName = formatVersion(seriesName);

// Check for special-case measurement formatting
const arg = getAggregateArg(seriesName);
if (arg) {
const slug = getMeasurementSlug(arg);

if (slug) {
seriesName = slug.toUpperCase();
}
}

// Strip equation prefix
if (maybeEquationAlias(seriesName)) {
seriesName = stripEquationPrefix(seriesName);
}

return seriesName;
return name;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {
TimeSeries,
TimeSeriesValueUnit,
} from 'sentry/views/dashboards/widgets/common/types';
import {formatTimeSeriesLabel} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesLabel';
import {formatTimeSeriesName} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesName';
import {FALLBACK_TYPE} from 'sentry/views/dashboards/widgets/timeSeriesWidget/settings';

Expand Down Expand Up @@ -62,21 +63,11 @@ export abstract class ContinuousTimeSeries<
* Continuous time series names need to be unique to disambiguate them from other series. We use both the `yAxis` and the `groupBy` to create the name. This makes it possible to pass in two different time series with the same `yAxis` as long as they have different `groupBy` information.
*/
get name(): string {
let name = `${this.timeSeries.yAxis}`;

if (this.timeSeries.groupBy?.length) {
name += ` : ${this.timeSeries.groupBy
?.map(groupBy => {
return `${groupBy.key} : ${groupBy.value}`;
})
.join(',')}`;
}

return name;
return formatTimeSeriesName(this.timeSeries);
}

get label(): string {
return this.config?.alias ?? formatTimeSeriesName(this.timeSeries);
return this.config?.alias ?? formatTimeSeriesLabel(this.timeSeries);
}

get isEmpty(): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
LegendSelection,
TimeSeries,
} from 'sentry/views/dashboards/widgets/common/types';
import {formatTimeSeriesName} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesName';
import {Area} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/area';
import {Bars} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/bars';
import {Line} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/line';
Expand Down Expand Up @@ -134,10 +135,16 @@ export function InsightsTimeSeriesWidget(props: InsightsTimeSeriesWidgetProps) {

yAxes.add(timeSeries.yAxis);

let alias = aliases?.[delayedTimeSeries.yAxis];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't always rely on the yAxis for alias. In the new timeSeries object, yAxis does not contain grouping information. To combat this I created formatTimeSeriesName, this is what the plottable now uses to generate it's name field, and we can also use that for the alias mapping.

const plottableName = formatTimeSeriesName(delayedTimeSeries);
if (aliases?.[plottableName]) {
alias = aliases?.[plottableName];
}

return new PlottableDataConstructor(delayedTimeSeries, {
color: COMMON_COLORS(theme)[delayedTimeSeries.yAxis],
color: COMMON_COLORS(theme)[plottableName],
stack: props.stacked && props.visualizationType === 'bar' ? 'all' : undefined,
alias: aliases?.[delayedTimeSeries.yAxis],
alias,
});
}),
...(props.extraPlottables ?? []),
Expand Down Expand Up @@ -288,5 +295,7 @@ const COMMON_COLORS = (theme: Theme): Record<string, string> => {
'performance_score(measurements.score.inp)': vitalColors[2],
'performance_score(measurements.score.cls)': vitalColors[3],
'performance_score(measurements.score.ttfb)': vitalColors[4],
'epm() : span.op : queue.publish': colors[1],
'epm() : span.op : queue.process': colors[2],
};
};
24 changes: 0 additions & 24 deletions static/app/views/insights/common/utils/renameDiscoverSeries.tsx

This file was deleted.

Loading
Loading