-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(dashboards): Render unfurl chart with dual Y axes for multi-aggregate widgets #115411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
DominikB2014
wants to merge
4
commits into
master
Choose a base branch
from
dominikbuszowiecki/fix/dashboards-unfurl-dual-yaxis
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+209
−137
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d0f1978
fix(dashboards): Render unfurl chart with dual Y axes for multi-aggre…
DominikB2014 8366d4e
fix: Preserve consensus null unit in assignPlottablesToYAxes
DominikB2014 b02c1c1
ref: Stop exporting unused YAxisAssignment type
DominikB2014 c7fb389
ref: Align shared y-axis helper with original dashboard implementation
DominikB2014 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
146 changes: 146 additions & 0 deletions
146
static/app/views/dashboards/widgets/timeSeriesWidget/assignPlottablesToYAxes.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, TimeSeriesValueUnit>; | ||
| }; | ||
|
|
||
| /** | ||
| * 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, | ||
| }; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.