diff --git a/static/app/chartcuterie/timeseries.tsx b/static/app/chartcuterie/timeseries.tsx index 3ebc2e72badb..366d5249a380 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 plottables = plottableEntries + .map(entry => entry.plottable) + .filter((plottable): plottable is Plottable => !!plottable); + + const {leftYAxisType, rightYAxisType, unitForType, getYAxisPosition} = + assignPlottablesToYAxes(plottables); + + 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 000000000000..8e15ae626b2e --- /dev/null +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx @@ -0,0 +1,146 @@ +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'; +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'; + +const {error} = Sentry.logger; + +type YAxisAssignment = { + /** + * Returns the side (`'left'` / `'right'`) a plottable should plot on. + * 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'; + leftYAxisDataTypes: string[]; + leftYAxisType: string; + rightYAxisDataTypes: string[]; + rightYAxisType: string | undefined; + unitForType: Record; +}; + +/** + * Partitions plottables across left/right Y axes by data type. Used by both + * `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 { + 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) { + 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, ofType => + uniq(ofType.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]; + }); + + const getYAxisPosition = (plottable: Plottable): 'left' | 'right' => { + 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 { + 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 6eac8736992a..1be0d7d6636f 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,11 +51,12 @@ 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; +const {warn} = Sentry.logger; export interface TimeSeriesWidgetVisualizationProps extends Partial { /** @@ -188,78 +185,11 @@ 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 {leftYAxisType, rightYAxisType, unitForType, getYAxisPosition} = + assignPlottablesToYAxes(props.plottables); const axisRangeProp = getAxisRange(props.axisRange) ?? 'auto'; @@ -551,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,