Skip to content
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.

Sign up for GitHub

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

[APM] Transaction breakdown charts #39531

Merged
merged 7 commits into from
Jul 2, 2019
Merged

[APM] Transaction breakdown charts #39531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2f2534f
[APM] Transaction breakdown graphs
dgieselaar Jun 21, 2019
a4149c6
Don't use .keyword now that mapping is correct
dgieselaar Jul 1, 2019
49afb52
Use better heuristic for determining noHits; wrap App in memo to prev…
dgieselaar Jul 1, 2019
52945d0
Remove nested panel around TransactionBreakdown
dgieselaar Jul 1, 2019
a7e701a
Fix display issues in charts
dgieselaar Jul 1, 2019
d72a3c6
Address review feedback
dgieselaar Jul 2, 2019
c808e71
Address additional review feedback
dgieselaar Jul 2, 2019
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
12 changes: 12 additions & 0 deletions x-pack/legacy/plugins/apm/common/__snapshots__/elasticsearch_fieldnames.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/apm/common/elasticsearch_fieldnames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ export const TRANSACTION_RESULT = 'transaction.result';
export const TRANSACTION_NAME = 'transaction.name';
export const TRANSACTION_ID = 'transaction.id';
export const TRANSACTION_SAMPLED = 'transaction.sampled';
export const TRANSACTION_BREAKDOWN_COUNT = 'transaction.breakdown.count';

export const TRACE_ID = 'trace.id';

export const SPAN_DURATION = 'span.duration.us';
export const SPAN_TYPE = 'span.type';
export const SPAN_SUBTYPE = 'span.subtype';
export const SPAN_SELF_TIME_SUM = 'span.self_time.sum.us';
export const SPAN_ACTION = 'span.action';
export const SPAN_NAME = 'span.name';
export const SPAN_ID = 'span.id';
Expand Down
19 changes: 11 additions & 8 deletions x-pack/legacy/plugins/apm/public/components/app/TransactionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { useLocation } from '../../../hooks/useLocation';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { FETCH_STATUS } from '../../../hooks/useFetcher';
import { TransactionBreakdown } from '../../shared/TransactionBreakdown';
import { ChartsTimeContextProvider } from '../../../context/ChartsTimeContext';

export function TransactionDetails() {
const location = useLocation();
Expand All @@ -42,16 +43,18 @@ export function TransactionDetails() {
</EuiTitle>
</ApmHeader>

<TransactionBreakdown />
<ChartsTimeContextProvider>
<TransactionBreakdown />

<EuiSpacer size="s" />
<EuiSpacer size="s" />

<TransactionCharts
hasMLJob={false}
charts={transactionDetailsChartsData}
urlParams={urlParams}
location={location}
/>
<TransactionCharts
hasMLJob={false}
charts={transactionDetailsChartsData}
urlParams={urlParams}
location={location}
/>
</ChartsTimeContextProvider>

<EuiHorizontalRule size="full" margin="l" />

Expand Down
19 changes: 11 additions & 8 deletions x-pack/legacy/plugins/apm/public/components/app/TransactionOverview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { useFetcher } from '../../../hooks/useFetcher';
import { getHasMLJob } from '../../../services/rest/ml';
import { history } from '../../../utils/history';
import { useLocation } from '../../../hooks/useLocation';
import { ChartsTimeContextProvider } from '../../../context/ChartsTimeContext';

interface Props {
urlParams: IUrlParams;
Expand Down Expand Up @@ -114,16 +115,18 @@ export function TransactionOverview({
</EuiFormRow>
) : null}

<TransactionBreakdown />
<ChartsTimeContextProvider>
<TransactionBreakdown initialIsOpen={true} />

<EuiSpacer size="s" />
<EuiSpacer size="s" />

<TransactionCharts
hasMLJob={hasMLJob}
charts={transactionOverviewCharts}
location={location}
urlParams={urlParams}
/>
<TransactionCharts
hasMLJob={hasMLJob}
charts={transactionOverviewCharts}
location={location}
urlParams={urlParams}
/>
</ChartsTimeContextProvider>

<EuiSpacer size="s" />

Expand Down
58 changes: 55 additions & 3 deletions ...ins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownGraph/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,62 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import React, { useMemo, useCallback } from 'react';
import numeral from '@elastic/numeral';
import { NOT_AVAILABLE_LABEL } from '../../../../../common/i18n';
import { Coordinate } from '../../../../../typings/timeseries';
import { TransactionLineChart } from '../../charts/TransactionCharts/TransactionLineChart';
import { asPercent } from '../../../../utils/formatters';
import { unit } from '../../../../style/variables';

const TransactionBreakdownGraph: React.FC<{}> = () => {
return <div />;
interface Props {
timeseries: Array<{
name: string;
color: string;
values: Array<{ x: number; y: number | null }>;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be consistent with the other charts we have (name -> title and values -> data).
As mentioned somewhere else I prefer values over data but might be a bigger job to change that everywhere.

Instead of having several different interfaces can we make a single timeseries interface that is re-used?

}>;
}

const TransactionBreakdownGraph: React.FC<Props> = props => {
const { timeseries } = props;

const series: React.ComponentProps<
typeof TransactionLineChart
>['series'] = useMemo(
() => {
return timeseries.map(timeseriesConfig => {
return {
title: timeseriesConfig.name,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this rename necessary? Shouldn't timeseriesConfig.name be timeseriesConfig.title in the first place? Same with timeseriesConfig.values - shouldn't that be timeseriesConfig.data in the first place?

I actually prefer values over data but then we should change that everywhere.

color: timeseriesConfig.color,
data: timeseriesConfig.values,
type: 'area',
hideLegend: true
};
}, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

the last arg {} isn't needed since there's no reference to this in the map callback function

Copy link
Member Author

Choose a reason for hiding this comment

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

aye, think this was a reduce before and I forgot to remove the second argument. thanks!

},
[timeseries]
);

const tickFormatY = useCallback((y: number | null) => {
return numeral(y || 0).format('0 %');
}, []);

const formatTooltipValue = useCallback((coordinate: Coordinate) => {
return coordinate.y !== null && coordinate.y !== undefined
? asPercent(coordinate.y, 1)
: NOT_AVAILABLE_LABEL;
}, []);

return (
<TransactionLineChart
series={series}
stacked={true}
tickFormatY={tickFormatY}
formatTooltipValue={formatTooltipValue}
yMax={1}
height={unit * 12}
/>
);
};

export { TransactionBreakdownGraph };
1 change: 0 additions & 1 deletion ...plugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownKpiList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { FORMATTERS } from '../../../../../infra/public/utils/formatters';
interface TransactionBreakdownKpi {
name: string;
percentage: number;
count: number;
color: string;
}

Expand Down
79 changes: 62 additions & 17 deletions x-pack/legacy/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
EuiSpacer,
EuiPanel
} from '@elastic/eui';
import { sortBy } from 'lodash';
import { i18n } from '@kbn/i18n';
import styled from 'styled-components';
import { useTransactionBreakdown } from '../../../hooks/useTransactionBreakdown';
Expand All @@ -38,32 +37,76 @@ const NoTransactionsTitle = styled.span`
font-weight: bold;
`;

const TransactionBreakdown: React.FC = () => {
const [showChart, setShowChart] = useState(false);
const TransactionBreakdown: React.FC<{
initialIsOpen?: boolean;
}> = ({ initialIsOpen }) => {
const [showChart, setShowChart] = useState(!!initialIsOpen);

const {
data,
status,
receivedDataDuringLifetime
} = useTransactionBreakdown();

const kpis = useMemo(
const kpis = data ? data.kpis : undefined;
const timeseriesPerSubtype = data ? data.timeseries_per_subtype : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I see this access checks a lot of places which makes the code a bit harder to read. What about setting a default response like we do in:

kibana/x-pack/legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts

Lines 13 to 15 in 868c712

const INITIAL_DATA: MetricsChartsByAgentAPIResponse = {
charts: []
};

Could be something like:

const INITIAL_DATA: TransactionBreakdownAPIResponse = {
      kpis: [],
      timeseries_per_subtype: null
    }


const legends = useMemo(
() => {
return data
? sortBy(data, 'name').map((breakdown, index) => {
return {
...breakdown,
color: COLORS[index % COLORS.length]
};
})
: null;
const names = kpis ? kpis.map(kpi => kpi.name).sort() : [];

return names.map((name, index) => {
return {
name,
color: COLORS[index % COLORS.length]
};
});
},
[data]
[kpis]
);

const sortedAndColoredKpis = useMemo(
() => {
if (!kpis) {
return null;
}

return legends.map(legend => {
const { color } = legend;

const breakdown = kpis.find(
b => b.name === legend.name
) as typeof kpis[0];
Copy link
Member

@sorenlouv sorenlouv Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What if no matches are found? Then breakdown will be undefined and cause a runtime error below - or is that not logically possible?


return {
...breakdown,
color
};
});
},
[kpis, legends]
);

const loading = status === FETCH_STATUS.LOADING || status === undefined;

const hasHits = data && data.length > 0;
const hasHits = data && data.kpis.length > 0;
const timeseries = useMemo(
() => {
if (!timeseriesPerSubtype) {
return [];
}
return legends.map(legend => {
const series = timeseriesPerSubtype[legend.name];

return {
name: legend.name,
values: series,
color: legend.color
};
});
},
[timeseriesPerSubtype, legends]
);

return receivedDataDuringLifetime ? (
<EuiPanel>
Expand All @@ -77,9 +120,11 @@ const TransactionBreakdown: React.FC = () => {
}}
/>
</EuiFlexItem>
{hasHits && kpis ? (
{hasHits && sortedAndColoredKpis ? (
<EuiFlexItem>
{kpis && <TransactionBreakdownKpiList kpis={kpis} />}
{sortedAndColoredKpis && (
<TransactionBreakdownKpiList kpis={sortedAndColoredKpis} />
)}
</EuiFlexItem>
) : (
!loading && (
Expand Down Expand Up @@ -114,7 +159,7 @@ const TransactionBreakdown: React.FC = () => {
)}
{showChart && hasHits ? (
<EuiFlexItem>
<TransactionBreakdownGraph />
<TransactionBreakdownGraph timeseries={timeseries} />
</EuiFlexItem>
) : null}
</EuiFlexGroup>
Expand Down
28 changes: 16 additions & 12 deletions x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/InteractivePlot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,22 @@ function getPointByX(serie, x) {

class InteractivePlot extends PureComponent {
getMarkPoints = hoverX => {
return this.props.series
.filter(serie =>
serie.data.some(point => point.x === hoverX && point.y != null)
)
.map(serie => {
const { x, y } = getPointByX(serie, hoverX) || {};
return {
x,
y,
color: serie.color
};
});
return (
this.props.series
.filter(serie =>
serie.data.some(point => point.x === hoverX && point.y != null)
)
.map(serie => {
const { x, y } = getPointByX(serie, hoverX) || {};
return {
x,
y,
color: serie.color
};
})
// needs to be reversed, as StaticPlot.js does the same
.reverse()
);
};

getTooltipPoints = hoverX => {
Expand Down
4 changes: 3 additions & 1 deletion x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/StaticPlot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class StaticPlot extends PureComponent {
curve={'curveMonotoneX'}
data={serie.data}
color={serie.color}
stack={serie.stack}
/>
);
case 'area':
Expand All @@ -53,8 +54,9 @@ class StaticPlot extends PureComponent {
curve={'curveMonotoneX'}
data={serie.data}
color={serie.color}
stroke={serie.color}
stroke={serie.stack ? 'rgba(0,0,0,0)' : serie.color}
Copy link
Member Author

Choose a reason for hiding this comment

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

stroke has to be transparent for a stacked area chart, because we stick a line chart on top of it to achieve the look that we want.

fill={serie.areaColor || rgba(serie.color, 0.3)}
stack={serie.stack}
/>
);
case 'areaMaxHeight':
Expand Down
Loading