From d0f19780da81742206401222dd213aadeb5593c7 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Tue, 12 May 2026 15:38:28 -0400 Subject: [PATCH 1/4] fix(dashboards): Render unfurl chart with dual Y axes for multi-aggregate widgets Multi-aggregate widgets (e.g. `count(span.duration)` + `avg(span.duration)`) previously rendered with a single Y axis in Slack unfurl charts. The chartcuterie config now reuses the dashboard's axis partitioning so unfurls match the UI. Extracts the partitioning into `assignPlottablesToYAxes` so the dashboard widget visualization and the chartcuterie unfurl chart share one implementation. --- static/app/chartcuterie/timeseries.tsx | 86 +++++++----- .../assignPlottablesToYAxes.tsx | 123 ++++++++++++++++++ .../timeSeriesWidgetVisualization.tsx | 89 ++----------- 3 files changed, 189 insertions(+), 109 deletions(-) create mode 100644 static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx diff --git a/static/app/chartcuterie/timeseries.tsx b/static/app/chartcuterie/timeseries.tsx index 3ebc2e72badbd0..65b5e02942d86f 100644 --- a/static/app/chartcuterie/timeseries.tsx +++ b/static/app/chartcuterie/timeseries.tsx @@ -6,11 +6,13 @@ import {XAxis} from 'sentry/components/charts/components/xAxis'; import {YAxis} from 'sentry/components/charts/components/yAxis'; import {DisplayType} from 'sentry/views/dashboards/types'; import type {TimeSeries} from 'sentry/views/dashboards/widgets/common/types'; +import {assignPlottablesToYAxes} from 'sentry/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes'; import {formatTimeSeriesLabel} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesLabel'; import {formatYAxisValue} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatYAxisValue'; import type {ContinuousTimeSeriesPlottingOptions} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries'; import {createPlottableFromTimeSeries} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/createPlottableFromTimeSeries'; import type {Plottable} from 'sentry/views/dashboards/widgets/timeSeriesWidget/plottables/plottable'; +import {FALLBACK_TYPE} from 'sentry/views/dashboards/widgets/timeSeriesWidget/settings'; import {DEFAULT_FONT_FAMILY} from './slack'; import type {RenderDescriptor} from './types'; @@ -22,17 +24,6 @@ import {ChartType} from './types'; const FONT_SIZE = 28; export const CHART_SIZE = {width: 1200, height: 400}; -/** - * Builds a y-axis axisLabel formatter from the first timeseries metadata. - */ -function makeYAxisFormatter(timeSeries: TimeSeries[]) { - const firstSeries = timeSeries[0]; - const valueType = firstSeries?.meta?.valueType ?? 'number'; - const valueUnit = firstSeries?.meta?.valueUnit; - - return (value: number) => formatYAxisValue(value, valueType, valueUnit ?? undefined); -} - export type TimeseriesChartData = { timeSeries: TimeSeries[]; type?: DisplayType; @@ -82,6 +73,22 @@ export function buildTimeseriesChartOption({ axisLabel: {fontSize: FONT_SIZE, fontFamily: DEFAULT_FONT_FAMILY}, }); + const makeYAxis = ( + type: string, + unit: string | undefined, + position: 'left' | 'right' + ) => + YAxis({ + theme, + splitNumber: 3, + position, + axisLabel: { + fontSize: FONT_SIZE, + fontFamily: DEFAULT_FONT_FAMILY, + formatter: (value: number) => formatYAxisValue(value, type, unit), + }, + }); + const defaults = { grid: Grid({left: 10, right: 10, bottom: 10, top: GRID_TOP_OFFSET}), backgroundColor: theme.tokens.background.primary, @@ -105,11 +112,6 @@ export function buildTimeseriesChartOption({ }, pageIconSize: FONT_SIZE * 0.6, }), - yAxis: YAxis({ - theme, - splitNumber: 3, - axisLabel: {fontSize: FONT_SIZE, fontFamily: DEFAULT_FONT_FAMILY}, - }), }; if (timeSeries.length === 0) { @@ -118,19 +120,10 @@ export function buildTimeseriesChartOption({ xAxis, useUTC: true, series: [], + yAxis: makeYAxis(FALLBACK_TYPE, undefined, 'left'), }; } - const yAxis = YAxis({ - theme, - splitNumber: 3, - axisLabel: { - fontSize: FONT_SIZE, - fontFamily: DEFAULT_FONT_FAMILY, - formatter: makeYAxisFormatter(timeSeries), - }, - }); - const hasGroups = timeSeries.some(ts => ts.groupBy && ts.groupBy.length > 0); // Grouped widgets stack in order, with the "Other" bucket pinned to a @@ -146,14 +139,43 @@ export function buildTimeseriesChartOption({ color.push(theme.tokens.content.secondary); } - const series = sorted.flatMap((ts, i) => { + // Build plottables up front so we can hand them to the shared y-axis + // partitioner. Mirrors the dashboard widget's dual-axis logic so unfurls + // render multi-aggregate widgets (e.g. `count` + `avg(duration)`) the same + // way the UI does. + const plottableEntries = sorted.map((ts, i) => ({ + ts, + color: color?.[i] ?? '', + plottable: createPlottable(ts, {color: color?.[i], hasGroups, index: i}), + })); + const realizedPlottables = plottableEntries + .map(entry => entry.plottable) + .filter((plottable): plottable is Plottable => !!plottable); + + const {leftYAxisType, rightYAxisType, unitForType, getYAxisPosition} = + assignPlottablesToYAxes(realizedPlottables); + + const leftYAxis = makeYAxis( + leftYAxisType, + unitForType[leftYAxisType] ?? undefined, + 'left' + ); + const rightYAxis = rightYAxisType + ? makeYAxis(rightYAxisType, unitForType[rightYAxisType] ?? undefined, 'right') + : undefined; + const yAxis = rightYAxis ? [leftYAxis, rightYAxis] : leftYAxis; + + const series = plottableEntries.flatMap(({ts, plottable, color: plottableColor}) => { + if (!plottable) { + return []; + } + const dataType = plottable.dataType ?? FALLBACK_TYPE; const plottingOptions: ContinuousTimeSeriesPlottingOptions = { - color: color?.[i] ?? '', - unit: ts.meta?.valueUnit ?? null, - yAxisPosition: 'left', + color: plottableColor, + unit: unitForType[dataType] ?? ts.meta?.valueUnit ?? null, + yAxisPosition: getYAxisPosition(plottable), }; - const plottable = createPlottable(ts, {color: color?.[i], hasGroups, index: i}); - return plottable?.toSeries(plottingOptions) ?? []; + return plottable.toSeries(plottingOptions); }); const extraSeries = extraPlottables.flatMap(plottable => diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx new file mode 100644 index 00000000000000..c44a996cff5606 --- /dev/null +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx @@ -0,0 +1,123 @@ +import groupBy from 'lodash/groupBy'; + +import {uniq} from 'sentry/utils/array/uniq'; +import type {AggregationOutputType} from 'sentry/utils/discover/fields'; +import type {TimeSeriesValueUnit} from 'sentry/views/dashboards/widgets/common/types'; + +import type {Plottable} from './plottables/plottable'; +import {FALLBACK_TYPE, FALLBACK_UNIT_FOR_FIELD_TYPE} from './settings'; + +export type YAxisAssignment = { + /** + * Returns the side (`'left'` / `'right'`) a plottable should plot on. + * Plottables whose data type wasn't seen during partitioning fall back to + * the left axis so callers always get a usable position. + */ + getYAxisPosition: (plottable: Plottable) => 'left' | 'right'; + /** + * Data types assigned to the left axis. The first declared type anchors + * this list; when only one data type is present everything is here. + */ + leftYAxisDataTypes: string[]; + /** + * Single representative type for the left axis. Falls back to + * {@link FALLBACK_TYPE} when the left axis must carry multiple types. + */ + leftYAxisType: string; + /** + * Data types assigned to the right axis. Empty when no right axis is + * needed. + */ + rightYAxisDataTypes: string[]; + /** + * Single representative type for the right axis, or `undefined` when no + * right axis is needed. + */ + rightYAxisType: string | undefined; + /** + * Representative unit per data type. Used to build the axis label + * formatters and to pass `unit` into each plottable's `toSeries` call. + */ + unitForType: Record; +}; + +/** + * Partitions plottables across left/right Y axes by data type. Used by both + * the dashboard widget UI and the Slack dashboards-widget unfurl chartcuterie + * chart so unfurls render multi-aggregate widgets (e.g. `count` + + * `avg(duration)`) the same way the UI does. + * + * Rules (mirrors the legacy in-line logic in + * `TimeSeriesWidgetVisualization`): + * + * - 1 unique data type -> single left axis + * - 2 unique data types -> split, one per side + * - 3+ types, first = fallback -> all on left (avoid a fallback right axis) + * - 3+ types otherwise -> first on left, the remainder share right + */ +export function assignPlottablesToYAxes(plottables: Plottable[]): YAxisAssignment { + // Unique data types in order of first appearance — the first one anchors + // the left axis. + const axisTypes: string[] = []; + for (const plottable of plottables) { + const dataType = plottable.dataType ?? FALLBACK_TYPE; + if (!axisTypes.includes(dataType)) { + axisTypes.push(dataType); + } + } + + let leftYAxisDataTypes: string[] = []; + let rightYAxisDataTypes: string[] = []; + if (axisTypes.length <= 1) { + leftYAxisDataTypes = axisTypes; + } else if (axisTypes.length === 2) { + leftYAxisDataTypes = axisTypes.slice(0, 1); + rightYAxisDataTypes = axisTypes.slice(1, 2); + } else if (axisTypes.at(0) === FALLBACK_TYPE) { + leftYAxisDataTypes = axisTypes; + } else { + leftYAxisDataTypes = axisTypes.slice(0, 1); + rightYAxisDataTypes = axisTypes.slice(1); + } + + const leftYAxisType = + leftYAxisDataTypes.length === 1 ? leftYAxisDataTypes[0]! : FALLBACK_TYPE; + const rightYAxisType = + rightYAxisDataTypes.length === 0 + ? undefined + : rightYAxisDataTypes.length === 1 + ? rightYAxisDataTypes[0]! + : FALLBACK_TYPE; + + // Pick a representative unit per data type. When plottables of the same + // type disagree, fall back to the type's canonical unit so the axis + // formatter stays consistent. + const plottablesByType = groupBy( + plottables, + plottable => plottable.dataType ?? FALLBACK_TYPE + ); + const unitForType: Record = {}; + for (const [type, ofType] of Object.entries(plottablesByType)) { + const units = uniq(ofType.map(plottable => plottable.dataUnit)); + if (units.length === 1 && units[0]) { + unitForType[type] = units[0]; + } else { + unitForType[type] = + FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType] ?? null; + } + } + + const getYAxisPosition = (plottable: Plottable): 'left' | 'right' => { + const dataType = plottable.dataType ?? FALLBACK_TYPE; + return rightYAxisDataTypes.includes(dataType) ? 'right' : 'left'; + }; + + return { + leftYAxisDataTypes, + rightYAxisDataTypes, + leftYAxisType, + rightYAxisType, + unitForType, + getYAxisPosition, + }; +} diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx index 6eac8736992a4d..a3ff033ae77a14 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx @@ -7,8 +7,6 @@ import type { TooltipFormatterCallback, TopLevelFormatterParams, } from 'echarts/types/dist/shared'; -import groupBy from 'lodash/groupBy'; -import mapValues from 'lodash/mapValues'; import sum from 'lodash/sum'; import unescape from 'lodash/unescape'; @@ -33,8 +31,6 @@ import type { ReactEchartsRef, } from 'sentry/types/echarts'; import {defined, escape} from 'sentry/utils'; -import {uniq} from 'sentry/utils/array/uniq'; -import type {AggregationOutputType} from 'sentry/utils/discover/fields'; import {RangeMap, type Range} from 'sentry/utils/number/rangeMap'; import {useLocation} from 'sentry/utils/useLocation'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -55,8 +51,9 @@ import {formatTooltipValue} from './formatters/formatTooltipValue'; import {formatXAxisTimestamp} from './formatters/formatXAxisTimestamp'; import {formatYAxisValue} from './formatters/formatYAxisValue'; import type {Plottable} from './plottables/plottable'; +import {assignPlottablesToYAxes} from './assignPlottablesToYAxes'; import {ReleaseSeries} from './releaseSeries'; -import {FALLBACK_TYPE, FALLBACK_UNIT_FOR_FIELD_TYPE} from './settings'; +import {FALLBACK_TYPE} from './settings'; import {TimeSeriesWidgetYAxis} from './timeSeriesWidgetYAxis'; const {error, warn} = Sentry.logger; @@ -188,78 +185,16 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati ...props.chartXRangeSelection, }); - const plottablesByType = groupBy(props.plottables, plottable => plottable.dataType); - - // Get unique axis types in order of first appearance, treating the first - // aggregate as primary. This avoids axis flipping when thresholds or other - // plottables inflate the count of a particular data type. - const axisTypes: string[] = []; - for (const plottable of props.plottables) { - if (plottable.dataType && !axisTypes.includes(plottable.dataType)) { - axisTypes.push(plottable.dataType); - } - } - - // Partition the types between the two axes - let leftYAxisDataTypes: string[] = []; - let rightYAxisDataTypes: string[] = []; - - if (axisTypes.length === 1) { - // The simplest case, there is just one type. Assign it to the left axis - leftYAxisDataTypes = axisTypes; - } else if (axisTypes.length === 2) { - // Also a simple case. If there are only two types, split them evenly - leftYAxisDataTypes = axisTypes.slice(0, 1); - rightYAxisDataTypes = axisTypes.slice(1, 2); - } else if (axisTypes.length > 2 && axisTypes.at(0) === FALLBACK_TYPE) { - // There are multiple types, and the first one is the fallback. Don't - // bother creating a second fallback axis, plot everything on the left - leftYAxisDataTypes = axisTypes; - } else { - // There are multiple types. Assign the first type to the left axis, - // the rest to the right axis - leftYAxisDataTypes = axisTypes.slice(0, 1); - rightYAxisDataTypes = axisTypes.slice(1); - } - - // The left Y axis might be responsible for 1 or more types. If there's just - // one, use that type. If it's responsible for more than 1 type, use the - // fallback type - const leftYAxisType = - leftYAxisDataTypes.length === 1 ? leftYAxisDataTypes.at(0)! : FALLBACK_TYPE; - - // The right Y axis might be responsible for 0, 1, or more types. If there are - // none, don't set a type at all. If there is 1, use that type. If there are - // two or more, use fallback type - const rightYAxisType = - rightYAxisDataTypes.length === 0 - ? undefined - : rightYAxisDataTypes.length === 1 - ? rightYAxisDataTypes.at(0) - : FALLBACK_TYPE; - - // Create a map of used units by plottable data type - const unitsByType = mapValues(plottablesByType, plottables => - uniq(plottables.map(plottable => plottable.dataUnit)) - ); - - // Narrow down to just one unit for each plottable data type - const unitForType = mapValues(unitsByType, (relevantUnits, type) => { - if (relevantUnits.length === 1) { - // All plottables of this type have the same unit - return relevantUnits[0]!; - } - - if (relevantUnits.length === 0) { - // None of the plottables of this type supplied a unit - return FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType]; - } - - // Plottables of this type has mismatched units. Return a fallback. It - // would also be acceptable to return the unit of the _first_ plottable, - // probably - return FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType]; - }); + // Partition plottables across left/right Y axes by data type. Shared with + // the Slack dashboards-widget unfurl chart so unfurls render multi-aggregate + // widgets (e.g. `count` + `avg(duration)`) the same way the UI does. + const { + leftYAxisDataTypes, + rightYAxisDataTypes, + leftYAxisType, + rightYAxisType, + unitForType, + } = assignPlottablesToYAxes(props.plottables); const axisRangeProp = getAxisRange(props.axisRange) ?? 'auto'; From 8366d4ec9d10e85e72c89efdaffca7080f15181b Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Tue, 12 May 2026 16:40:30 -0400 Subject: [PATCH 2/4] fix: Preserve consensus null unit in assignPlottablesToYAxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `null` `dataUnit` is a legitimate "unitless" value. Previously the truthiness check on `units[0]` dropped a consensus `null` through to the fallback unit lookup (e.g. `duration` → `millisecond`), diverging from the dashboard's original behavior. --- .../widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx index c44a996cff5606..c902ae57bb4bfd 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx @@ -99,8 +99,10 @@ export function assignPlottablesToYAxes(plottables: Plottable[]): YAxisAssignmen const unitForType: Record = {}; for (const [type, ofType] of Object.entries(plottablesByType)) { const units = uniq(ofType.map(plottable => plottable.dataUnit)); - if (units.length === 1 && units[0]) { - unitForType[type] = units[0]; + if (units.length === 1) { + // Preserve the agreed-upon value even when it's `null` (a legitimate + // "unitless" value). The original dashboard logic returned it as-is. + unitForType[type] = units[0]!; } else { unitForType[type] = FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType] ?? null; From b02c1c1b3841f33228c12616f9634b4cbc54fd84 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Tue, 12 May 2026 16:41:20 -0400 Subject: [PATCH 3/4] ref: Stop exporting unused YAxisAssignment type --- .../widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx index c902ae57bb4bfd..77b412807bce39 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx @@ -7,7 +7,7 @@ import type {TimeSeriesValueUnit} from 'sentry/views/dashboards/widgets/common/t import type {Plottable} from './plottables/plottable'; import {FALLBACK_TYPE, FALLBACK_UNIT_FOR_FIELD_TYPE} from './settings'; -export type YAxisAssignment = { +type YAxisAssignment = { /** * Returns the side (`'left'` / `'right'`) a plottable should plot on. * Plottables whose data type wasn't seen during partitioning fall back to From c7fb38969fa492f4c0c20955176fcc8c6be56e28 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Tue, 12 May 2026 16:47:15 -0400 Subject: [PATCH 4/4] ref: Align shared y-axis helper with original dashboard implementation Matches the partitioning, comments, and Sentry error logging from the in-line implementation that used to live in `TimeSeriesWidgetVisualization`. The visualization now delegates yAxisPosition resolution to `assignPlottablesToYAxes` instead of duplicating the if/else block, and the chartcuterie call site renames `realizedPlottables` to `plottables`. --- static/app/chartcuterie/timeseries.tsx | 4 +- .../assignPlottablesToYAxes.tsx | 143 ++++++++++-------- .../timeSeriesWidgetVisualization.tsx | 39 +---- 3 files changed, 88 insertions(+), 98 deletions(-) diff --git a/static/app/chartcuterie/timeseries.tsx b/static/app/chartcuterie/timeseries.tsx index 65b5e02942d86f..366d5249a38083 100644 --- a/static/app/chartcuterie/timeseries.tsx +++ b/static/app/chartcuterie/timeseries.tsx @@ -148,12 +148,12 @@ export function buildTimeseriesChartOption({ color: color?.[i] ?? '', plottable: createPlottable(ts, {color: color?.[i], hasGroups, index: i}), })); - const realizedPlottables = plottableEntries + const plottables = plottableEntries .map(entry => entry.plottable) .filter((plottable): plottable is Plottable => !!plottable); const {leftYAxisType, rightYAxisType, unitForType, getYAxisPosition} = - assignPlottablesToYAxes(realizedPlottables); + assignPlottablesToYAxes(plottables); const leftYAxis = makeYAxis( leftYAxisType, diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx index 77b412807bce39..8e15ae626b2e5a 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx @@ -1,4 +1,6 @@ +import * as Sentry from '@sentry/react'; import groupBy from 'lodash/groupBy'; +import mapValues from 'lodash/mapValues'; import {uniq} from 'sentry/utils/array/uniq'; import type {AggregationOutputType} from 'sentry/utils/discover/fields'; @@ -7,111 +9,130 @@ import type {TimeSeriesValueUnit} from 'sentry/views/dashboards/widgets/common/t import type {Plottable} from './plottables/plottable'; import {FALLBACK_TYPE, FALLBACK_UNIT_FOR_FIELD_TYPE} from './settings'; +const {error} = Sentry.logger; + type YAxisAssignment = { /** * Returns the side (`'left'` / `'right'`) a plottable should plot on. - * Plottables whose data type wasn't seen during partitioning fall back to - * the left axis so callers always get a usable position. + * Logs to Sentry when a plottable's data type wasn't seen during + * partitioning, mirroring the original in-line behavior. */ getYAxisPosition: (plottable: Plottable) => 'left' | 'right'; - /** - * Data types assigned to the left axis. The first declared type anchors - * this list; when only one data type is present everything is here. - */ leftYAxisDataTypes: string[]; - /** - * Single representative type for the left axis. Falls back to - * {@link FALLBACK_TYPE} when the left axis must carry multiple types. - */ leftYAxisType: string; - /** - * Data types assigned to the right axis. Empty when no right axis is - * needed. - */ rightYAxisDataTypes: string[]; - /** - * Single representative type for the right axis, or `undefined` when no - * right axis is needed. - */ rightYAxisType: string | undefined; - /** - * Representative unit per data type. Used to build the axis label - * formatters and to pass `unit` into each plottable's `toSeries` call. - */ unitForType: Record; }; /** * Partitions plottables across left/right Y axes by data type. Used by both - * the dashboard widget UI and the Slack dashboards-widget unfurl chartcuterie - * chart so unfurls render multi-aggregate widgets (e.g. `count` + - * `avg(duration)`) the same way the UI does. - * - * Rules (mirrors the legacy in-line logic in - * `TimeSeriesWidgetVisualization`): - * - * - 1 unique data type -> single left axis - * - 2 unique data types -> split, one per side - * - 3+ types, first = fallback -> all on left (avoid a fallback right axis) - * - 3+ types otherwise -> first on left, the remainder share right + * `TimeSeriesWidgetVisualization` and the Slack dashboards-widget unfurl + * chartcuterie chart so unfurls render multi-aggregate widgets the same way + * the UI does. */ export function assignPlottablesToYAxes(plottables: Plottable[]): YAxisAssignment { - // Unique data types in order of first appearance — the first one anchors - // the left axis. + const plottablesByType = groupBy(plottables, plottable => plottable.dataType); + + // Get unique axis types in order of first appearance, treating the first + // aggregate as primary. This avoids axis flipping when thresholds or other + // plottables inflate the count of a particular data type. const axisTypes: string[] = []; for (const plottable of plottables) { - const dataType = plottable.dataType ?? FALLBACK_TYPE; - if (!axisTypes.includes(dataType)) { - axisTypes.push(dataType); + if (plottable.dataType && !axisTypes.includes(plottable.dataType)) { + axisTypes.push(plottable.dataType); } } + // Partition the types between the two axes let leftYAxisDataTypes: string[] = []; let rightYAxisDataTypes: string[] = []; - if (axisTypes.length <= 1) { + + if (axisTypes.length === 1) { + // The simplest case, there is just one type. Assign it to the left axis leftYAxisDataTypes = axisTypes; } else if (axisTypes.length === 2) { + // Also a simple case. If there are only two types, split them evenly leftYAxisDataTypes = axisTypes.slice(0, 1); rightYAxisDataTypes = axisTypes.slice(1, 2); - } else if (axisTypes.at(0) === FALLBACK_TYPE) { + } else if (axisTypes.length > 2 && axisTypes.at(0) === FALLBACK_TYPE) { + // There are multiple types, and the first one is the fallback. Don't + // bother creating a second fallback axis, plot everything on the left leftYAxisDataTypes = axisTypes; } else { + // There are multiple types. Assign the first type to the left axis, + // the rest to the right axis leftYAxisDataTypes = axisTypes.slice(0, 1); rightYAxisDataTypes = axisTypes.slice(1); } + // The left Y axis might be responsible for 1 or more types. If there's just + // one, use that type. If it's responsible for more than 1 type, use the + // fallback type const leftYAxisType = - leftYAxisDataTypes.length === 1 ? leftYAxisDataTypes[0]! : FALLBACK_TYPE; + leftYAxisDataTypes.length === 1 ? leftYAxisDataTypes.at(0)! : FALLBACK_TYPE; + + // The right Y axis might be responsible for 0, 1, or more types. If there are + // none, don't set a type at all. If there is 1, use that type. If there are + // two or more, use fallback type const rightYAxisType = rightYAxisDataTypes.length === 0 ? undefined : rightYAxisDataTypes.length === 1 - ? rightYAxisDataTypes[0]! + ? rightYAxisDataTypes.at(0) : FALLBACK_TYPE; - // Pick a representative unit per data type. When plottables of the same - // type disagree, fall back to the type's canonical unit so the axis - // formatter stays consistent. - const plottablesByType = groupBy( - plottables, - plottable => plottable.dataType ?? FALLBACK_TYPE + // Create a map of used units by plottable data type + const unitsByType = mapValues(plottablesByType, ofType => + uniq(ofType.map(plottable => plottable.dataUnit)) ); - const unitForType: Record = {}; - for (const [type, ofType] of Object.entries(plottablesByType)) { - const units = uniq(ofType.map(plottable => plottable.dataUnit)); - if (units.length === 1) { - // Preserve the agreed-upon value even when it's `null` (a legitimate - // "unitless" value). The original dashboard logic returned it as-is. - unitForType[type] = units[0]!; - } else { - unitForType[type] = - FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType] ?? null; + + // Narrow down to just one unit for each plottable data type + const unitForType = mapValues(unitsByType, (relevantUnits, type) => { + if (relevantUnits.length === 1) { + // All plottables of this type have the same unit + return relevantUnits[0]!; } - } + + if (relevantUnits.length === 0) { + // None of the plottables of this type supplied a unit + return FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType]; + } + + // Plottables of this type has mismatched units. Return a fallback. It + // would also be acceptable to return the unit of the _first_ plottable, + // probably + return FALLBACK_UNIT_FOR_FIELD_TYPE[type as AggregationOutputType]; + }); const getYAxisPosition = (plottable: Plottable): 'left' | 'right' => { - const dataType = plottable.dataType ?? FALLBACK_TYPE; - return rightYAxisDataTypes.includes(dataType) ? 'right' : 'left'; + let yAxisPosition: 'left' | 'right' = 'left'; + + if (leftYAxisDataTypes.includes(plottable.dataType)) { + // This plottable is assigned to the left axis + yAxisPosition = 'left'; + } else if (rightYAxisDataTypes.includes(plottable.dataType)) { + // This plottable is assigned to the right axis + yAxisPosition = 'right'; + } else { + // This plottable's type isn't assignned to either axis! Mysterious. + // There's no graceful way to handle this. + Sentry.withScope(scope => { + const message = + '`TimeSeriesWidgetVisualization` Could not assign Plottable to an axis'; + + scope.setFingerprint(['could-not-assign-plottable-to-an-axis']); + Sentry.captureException(new Error(message)); + + error(message, { + dataType: plottable.dataType, + leftAxisType: leftYAxisType, + rightAxisType: rightYAxisType, + }); + }); + } + + return yAxisPosition; }; return { diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx index a3ff033ae77a14..1be0d7d6636f83 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx @@ -56,7 +56,7 @@ import {ReleaseSeries} from './releaseSeries'; import {FALLBACK_TYPE} from './settings'; import {TimeSeriesWidgetYAxis} from './timeSeriesWidgetYAxis'; -const {error, warn} = Sentry.logger; +const {warn} = Sentry.logger; export interface TimeSeriesWidgetVisualizationProps extends Partial { /** @@ -188,13 +188,8 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati // Partition plottables across left/right Y axes by data type. Shared with // the Slack dashboards-widget unfurl chart so unfurls render multi-aggregate // widgets (e.g. `count` + `avg(duration)`) the same way the UI does. - const { - leftYAxisDataTypes, - rightYAxisDataTypes, - leftYAxisType, - rightYAxisType, - unitForType, - } = assignPlottablesToYAxes(props.plottables); + const {leftYAxisType, rightYAxisType, unitForType, getYAxisPosition} = + assignPlottablesToYAxes(props.plottables); const axisRangeProp = getAxisRange(props.axisRange) ?? 'auto'; @@ -486,36 +481,10 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati seriesColorIndex += 1; } - let yAxisPosition: 'left' | 'right' = 'left'; - - if (leftYAxisDataTypes.includes(plottable.dataType)) { - // This plottable is assigned to the left axis - yAxisPosition = 'left'; - } else if (rightYAxisDataTypes.includes(plottable.dataType)) { - // This plottable is assigned to the right axis - yAxisPosition = 'right'; - } else { - // This plottable's type isn't assignned to either axis! Mysterious. - // There's no graceful way to handle this. - Sentry.withScope(scope => { - const message = - '`TimeSeriesWidgetVisualization` Could not assign Plottable to an axis'; - - scope.setFingerprint(['could-not-assign-plottable-to-an-axis']); - Sentry.captureException(new Error(message)); - - error(message, { - dataType: plottable.dataType, - leftAxisType: leftYAxisType, - rightAxisType: rightYAxisType, - }); - }); - } - // TODO: Type checking would be welcome here, but `plottingOptions` is unknown, since it depends on the implementation of the `Plottable` interface const seriesOfPlottable = plottable.toSeries({ color, - yAxisPosition, + yAxisPosition: getYAxisPosition(plottable), unit: unitForType[plottable.dataType ?? FALLBACK_TYPE], theme, maxOffset: thresholdMaxOffset,