[APM] Adding comparison to transactions table #91435
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| AreaSeries, | ||
| Chart, | ||
| CurveType, | ||
| LineSeries, | ||
| ScaleType, | ||
| Settings, | ||
| } from '@elastic/charts'; | ||
|
|
@@ -20,8 +21,9 @@ import { Coordinate } from '../../../../../typings/timeseries'; | |
| import { useChartTheme } from '../../../../../../observability/public'; | ||
| import { px, unit } from '../../../../style/variables'; | ||
| import { useTheme } from '../../../../hooks/use_theme'; | ||
| import { getComparisonChartTheme } from '../../time_comparison/get_time_range_comparison'; | ||
|
|
||
| export type Color = | ||
cauemarcondes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| | 'euiColorVis0' | ||
| | 'euiColorVis1' | ||
| | 'euiColorVis2' | ||
|
|
@@ -33,19 +35,27 @@ type Color = | |
| | 'euiColorVis8' | ||
| | 'euiColorVis9'; | ||
|
|
||
| function isEmptyTimeseries(series: Coordinate[]) { | ||
| return series.every((point) => point.y === null); | ||
| } | ||
|
|
||
| export function SparkPlot({ | ||
| color, | ||
| series, | ||
| comparisonSeries = [], | ||
| valueLabel, | ||
| compact, | ||
| }: { | ||
| color: Color; | ||
| series?: Coordinate[] | null; | ||
| valueLabel: React.ReactNode; | ||
| compact?: boolean; | ||
| comparisonSeries?: Coordinate[]; | ||
| }) { | ||
| const theme = useTheme(); | ||
| const defaultChartTheme = useChartTheme(); | ||
| const comparisonChartTheme = getComparisonChartTheme(theme); | ||
| const hasComparisonSeries = !!comparisonSeries?.length; | ||
|
|
||
| const sparkplotChartTheme = merge({}, defaultChartTheme, { | ||
| chartMargins: { left: 0, right: 0, top: 0, bottom: 0 }, | ||
|
|
@@ -55,6 +65,7 @@ export function SparkPlot({ | |
| areaSeriesStyle: { | ||
| point: { opacity: 0 }, | ||
| }, | ||
| ...(hasComparisonSeries ? comparisonChartTheme : {}), | ||
| }); | ||
|
|
||
| const colorValue = theme.eui[color]; | ||
|
|
@@ -64,10 +75,12 @@ export function SparkPlot({ | |
| width: compact ? px(unit * 3) : px(unit * 4), | ||
| }; | ||
|
|
||
| const SparkelineSeries = hasComparisonSeries ? LineSeries : AreaSeries; | ||
cauemarcondes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return ( | ||
| <EuiFlexGroup gutterSize="m" responsive={false}> | ||
| <EuiFlexItem grow={false}> | ||
| {!series || isEmptyTimeseries(series) ? ( | ||
cauemarcondes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <div | ||
| style={{ | ||
| ...chartSize, | ||
|
|
@@ -85,8 +98,8 @@ export function SparkPlot({ | |
| showLegend={false} | ||
| tooltip="none" | ||
| /> | ||
| <SparkelineSeries | ||
|
There was a problem hiding this comment. This can't be a LineSeries all the time (or an area series)? What changes? There was a problem hiding this comment. This is necessary because when comparison is enabled/visible we should change the current period sparkline to LineSeries and show the previous period with AreaSeries. There's no technical problem with changing all sparklines to LineSeries, but all tables that yet don't have comparison implemented would change to a LineSeries, if @formgeist agrees I'm fine too. There was a problem hiding this comment. The thoughts behind the mixed chart types was primarily to create difference in the sparklines between the current and previous time periods. I think if we keep to either line or area, the comparison vanishes a bit because of the size of the chart. @cauemarcondes is it easy to exemplify the two charts (line or area)? There was a problem hiding this comment. To be clear, I am fine with mixing line and area. I was just wondering why it's necessary for the current period to switch, and why it can't always be either a line or an area series. There was a problem hiding this comment.
Because there are some places where the spakline is used and it doesn't have comparison data, like on the Service Inventory page table. If I always show it as a LineSeries it would change this table too, that's why I asked @formgeist . |
||
| id="sparkelineSeries" | ||
| xScaleType={ScaleType.Time} | ||
| yScaleType={ScaleType.Linear} | ||
| xAccessor={'x'} | ||
|
|
@@ -95,6 +108,18 @@ export function SparkPlot({ | |
| color={colorValue} | ||
| curve={CurveType.CURVE_MONOTONE_X} | ||
| /> | ||
| {hasComparisonSeries && ( | ||
| <AreaSeries | ||
| id="comparisonSeries" | ||
| xScaleType={ScaleType.Time} | ||
| yScaleType={ScaleType.Linear} | ||
| xAccessor={'x'} | ||
| yAccessors={['y']} | ||
| data={comparisonSeries} | ||
| color={theme.eui.euiColorLightestShade} | ||
| curve={CurveType.CURVE_MONOTONE_X} | ||
| /> | ||
| )} | ||
| </Chart> | ||
| )} | ||
| </EuiFlexItem> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be worth trying to figure out if we can move calculating the visible transaction groups into the useFetcher call as well, to see if we no longer need to disable
react-hooks/exhaustive-deps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgieselaar I can calculate the visible transaction groups inside useFetcher, but I'll still need the
react-hooks/exhaustive-deps, becausecomparisonTypeneeds to be listed on its dependencies, even though it is not used, to trigger the comparison API call when it's changed.