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

Jump to bottom

[APM] Remove additional "No data" message and re-ordering charts #75399

Merged
merged 19 commits into from
Sep 1, 2020
Merged

[APM] Remove additional "No data" message and re-ordering charts #75399

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
624b2a3
removing extra message
cauemarcondes Aug 18, 2020
37410ce
adding x-axis values when no data is available
cauemarcondes Aug 19, 2020
6cef87d
reordering charts
cauemarcondes Aug 19, 2020
37b5c47
fixing internationalization
cauemarcondes Aug 19, 2020
c713db9
fixing transaction RUM agent order
cauemarcondes Aug 19, 2020
1868699
Merge branch 'master' of github.com:elastic/kibana into apm-chart-rem…
cauemarcondes Aug 20, 2020
31ac18d
addressing PR comment
cauemarcondes Aug 20, 2020
911e4bd
Merge branch 'master' into apm-chart-remove-msg
elasticmachine Aug 24, 2020
263fdce
removing kpis list and show it in the chart legend instead
cauemarcondes Aug 25, 2020
2f907f1
fixing API test
cauemarcondes Aug 25, 2020
3eb4f5f
Merge branch 'master' into apm-chart-remove-msg
elasticmachine Aug 26, 2020
2caea0c
Merge branch 'master' into apm-chart-remove-msg
elasticmachine Aug 27, 2020
eae5696
Merge branch 'master' into apm-chart-remove-msg
elasticmachine Aug 31, 2020
a807f06
moving asPercent to the common directory
cauemarcondes Aug 31, 2020
799e1fb
fixing api test
cauemarcondes Aug 31, 2020
5ab6936
Merge branch 'master' of github.com:elastic/kibana into apm-chart-rem…
cauemarcondes Sep 1, 2020
d960841
fixing unit test
cauemarcondes Sep 1, 2020
04bfdd0
removing unused prop
cauemarcondes Sep 1, 2020
c4f5968
fixing unit test
cauemarcondes Sep 1, 2020
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
38 changes: 12 additions & 26 deletions x-pack/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 @@ -5,32 +5,29 @@
*/

import {
EuiFlexGroup,
EuiFlexItem,
EuiHorizontalRule,
EuiPanel,
EuiSpacer,
EuiTitle,
EuiHorizontalRule,
EuiFlexGroup,
EuiFlexItem,
} from '@elastic/eui';
import React, { useMemo } from 'react';
import { EuiFlexGrid } from '@elastic/eui';
import { useTrackPageview } from '../../../../../observability/public';
import { Projection } from '../../../../common/projections';
import { ChartsSyncContextProvider } from '../../../context/ChartsSyncContext';
import { FETCH_STATUS } from '../../../hooks/useFetcher';
import { useLocation } from '../../../hooks/useLocation';
import { useTransactionCharts } from '../../../hooks/useTransactionCharts';
import { useTransactionDistribution } from '../../../hooks/useTransactionDistribution';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { useWaterfall } from '../../../hooks/useWaterfall';
import { TransactionCharts } from '../../shared/charts/TransactionCharts';
import { ApmHeader } from '../../shared/ApmHeader';
import { TransactionCharts } from '../../shared/charts/TransactionCharts';
import { HeightRetainer } from '../../shared/HeightRetainer';
import { LocalUIFilters } from '../../shared/LocalUIFilters';
import { TransactionDistribution } from './Distribution';
import { WaterfallWithSummmary } from './WaterfallWithSummmary';
import { useLocation } from '../../../hooks/useLocation';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { FETCH_STATUS } from '../../../hooks/useFetcher';
import { TransactionBreakdown } from '../../shared/TransactionBreakdown';
import { ChartsSyncContextProvider } from '../../../context/ChartsSyncContext';
import { useTrackPageview } from '../../../../../observability/public';
import { Projection } from '../../../../common/projections';
import { LocalUIFilters } from '../../shared/LocalUIFilters';
import { HeightRetainer } from '../../shared/HeightRetainer';
import { ErroneousTransactionsRateChart } from '../../shared/charts/ErroneousTransactionsRateChart';

export function TransactionDetails() {
const location = useLocation();
Expand Down Expand Up @@ -86,17 +83,6 @@ export function TransactionDetails() {
</EuiFlexItem>
<EuiFlexItem grow={7}>
<ChartsSyncContextProvider>
<EuiFlexGrid columns={2} gutterSize="s">
<EuiFlexItem>
<TransactionBreakdown />
</EuiFlexItem>
<EuiFlexItem>
<ErroneousTransactionsRateChart />
</EuiFlexItem>
</EuiFlexGrid>

<EuiSpacer size="s" />

<TransactionCharts
charts={transactionChartsData}
urlParams={urlParams}
Expand Down
50 changes: 18 additions & 32 deletions x-pack/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 @@ -5,40 +5,37 @@
*/

import {
EuiPanel,
EuiSpacer,
EuiTitle,
EuiCallOut,
EuiCode,
EuiFlexGroup,
EuiFlexItem,
EuiHorizontalRule,
EuiCallOut,
EuiCode,
EuiPanel,
EuiSpacer,
EuiTitle,
} from '@elastic/eui';
import { Location } from 'history';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import { Location } from 'history';
import { first } from 'lodash';
import React, { useMemo } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFlexGrid } from '@elastic/eui';
import { useTransactionList } from '../../../hooks/useTransactionList';
import { useTransactionCharts } from '../../../hooks/useTransactionCharts';
import { useTrackPageview } from '../../../../../observability/public';
import { Projection } from '../../../../common/projections';
import { ChartsSyncContextProvider } from '../../../context/ChartsSyncContext';
import { IUrlParams } from '../../../context/UrlParamsContext/types';
import { useLocation } from '../../../hooks/useLocation';
import { useServiceTransactionTypes } from '../../../hooks/useServiceTransactionTypes';
import { useTransactionCharts } from '../../../hooks/useTransactionCharts';
import { useTransactionList } from '../../../hooks/useTransactionList';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { history } from '../../../utils/history';
import { TransactionCharts } from '../../shared/charts/TransactionCharts';
import { ErroneousTransactionsRateChart } from '../../shared/charts/ErroneousTransactionsRateChart';
import { TransactionBreakdown } from '../../shared/TransactionBreakdown';
import { TransactionList } from './List';
import { ElasticDocsLink } from '../../shared/Links/ElasticDocsLink';
import { useRedirect } from './useRedirect';
import { history } from '../../../utils/history';
import { useLocation } from '../../../hooks/useLocation';
import { ChartsSyncContextProvider } from '../../../context/ChartsSyncContext';
import { useTrackPageview } from '../../../../../observability/public';
import { fromQuery, toQuery } from '../../shared/Links/url_helpers';
import { LocalUIFilters } from '../../shared/LocalUIFilters';
import { Projection } from '../../../../common/projections';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { useServiceTransactionTypes } from '../../../hooks/useServiceTransactionTypes';
import { TransactionTypeFilter } from '../../shared/LocalUIFilters/TransactionTypeFilter';
import { TransactionList } from './List';
import { useRedirect } from './useRedirect';

function getRedirectLocation({
urlParams,
Expand Down Expand Up @@ -127,17 +124,6 @@ export function TransactionOverview() {
</EuiFlexItem>
<EuiFlexItem grow={7}>
<ChartsSyncContextProvider>
<EuiFlexGrid columns={2} gutterSize="s">
<EuiFlexItem>
<TransactionBreakdown />
</EuiFlexItem>
<EuiFlexItem>
<ErroneousTransactionsRateChart />
</EuiFlexItem>
</EuiFlexGrid>

<EuiSpacer size="s" />

<TransactionCharts
charts={transactionCharts}
location={location}
Expand Down
29 changes: 20 additions & 9 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,19 +4,21 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useMemo } from 'react';
import { throttle } from 'lodash';
import React, { useMemo } from 'react';
import { useUiTracker } from '../../../../../../observability/public';
import { NOT_AVAILABLE_LABEL } from '../../../../../common/i18n';
import { Coordinate, TimeSeries } from '../../../../../typings/timeseries';
import { Maybe } from '../../../../../typings/common';
import { TransactionLineChart } from '../../charts/TransactionCharts/TransactionLineChart';
import { Coordinate, TimeSeries } from '../../../../../typings/timeseries';
import { useUrlParams } from '../../../../hooks/useUrlParams';
import { asPercent } from '../../../../utils/formatters';
import { unit } from '../../../../style/variables';
import { isValidCoordinateValue } from '../../../../utils/isValidCoordinateValue';
import { useUiTracker } from '../../../../../../observability/public';
import { getEmptySeries } from '../../charts/CustomPlot/getEmptySeries';
import { TransactionLineChart } from '../../charts/TransactionCharts/TransactionLineChart';

interface Props {
timeseries: TimeSeries[];
noHits: boolean;
}

const tickFormatY = (y: Maybe<number>) => {
Expand All @@ -29,24 +31,33 @@ const formatTooltipValue = (coordinate: Coordinate) => {
: NOT_AVAILABLE_LABEL;
};

function TransactionBreakdownGraph(props: Props) {
const { timeseries } = props;
function TransactionBreakdownGraph({ timeseries, noHits }: Props) {
const { urlParams } = useUrlParams();
const { rangeFrom, rangeTo } = urlParams;
const trackApmEvent = useUiTracker({ app: 'apm' });
const handleHover = useMemo(
() =>
throttle(() => trackApmEvent({ metric: 'hover_breakdown_chart' }), 60000),
[trackApmEvent]
);

const emptySeries =
rangeFrom && rangeTo
? getEmptySeries(
new Date(rangeFrom).getTime(),
new Date(rangeTo).getTime()
)
: [];

return (
<TransactionLineChart
series={timeseries}
series={noHits ? emptySeries : timeseries}
tickFormatY={tickFormatY}
formatTooltipValue={formatTooltipValue}
yMax={1}
height={unit * 12}
stacked={true}
onHover={handleHover}
visibleLegendCount={10}
/>
);
}
Expand Down
75 changes: 0 additions & 75 deletions ...plugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownKpiList.tsx
View file
Open in desktop

This file was deleted.

34 changes: 12 additions & 22 deletions x-pack/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 @@ -3,28 +3,19 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import {
EuiFlexGroup,
EuiFlexItem,
EuiPanel,
EuiText,
EuiTitle,
} from '@elastic/eui';
import { EuiFlexGroup, EuiFlexItem, EuiPanel, EuiTitle } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React from 'react';
import { isEmpty } from 'lodash';
import { FETCH_STATUS } from '../../../hooks/useFetcher';
import { useTransactionBreakdown } from '../../../hooks/useTransactionBreakdown';
import { TransactionBreakdownGraph } from './TransactionBreakdownGraph';
import { TransactionBreakdownKpiList } from './TransactionBreakdownKpiList';

const emptyMessage = i18n.translate('xpack.apm.transactionBreakdown.noData', {
defaultMessage: 'No data within this time range.',
});
import { asPercent } from '../../../utils/formatters';

function TransactionBreakdown() {
const { data, status } = useTransactionBreakdown();
const { kpis, timeseries } = data;
const noHits = data.kpis.length === 0 && status === FETCH_STATUS.SUCCESS;
const { timeseries } = data;
const noHits = isEmpty(timeseries) && status === FETCH_STATUS.SUCCESS;

return (
<EuiPanel>
Expand All @@ -39,14 +30,13 @@ function TransactionBreakdown() {
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem grow={false}>
{noHits ? (
<EuiText>{emptyMessage}</EuiText>
) : (
<TransactionBreakdownKpiList kpis={kpis} />
)}
</EuiFlexItem>
<EuiFlexItem grow={false}>
<TransactionBreakdownGraph timeseries={timeseries} />
<TransactionBreakdownGraph
timeseries={timeseries.map((serie) => ({
...serie,
legendValue: asPercent(serie.legendValue, 1),
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for overwriting series here, and not where it's defined?

kibana/x-pack/plugins/apm/server/lib/transactions/breakdown/index.ts

Lines 211 to 217 in 63265b6

const timeseries = kpis.map((kpi) => ({
title: kpi.name,
color: kpi.color,
type: 'areaStacked',
data: timeseriesPerSubtype[kpi.name],
hideLegend: true,
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asPercent is defined in /public and I think the server doesn't have to know how the UI is going to show the value, it should only provide the value. I have my concerns about color, type and hideLegend, maybe they should all be removed from the server and be defined only in the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #70161 in the backlog.

Copy link
Member

@sorenlouv sorenlouv Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the server doesn't have to know how the UI is going to show the value, it should only provide the value.

It depends. Are there any drawbacks to defining it on the server? I see clear drawbacks in defining the chart config across multiple files.
That being said, if it's too much of a hassle because it'll require you to move asPercent and its dependencies to the server I'm fine with leaving it as-is.

@smith Regarding #70161: I'd be surprised if we don't have a way to access the advanced settings from the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input @sqren, the only drawback I can see here is that I'd have to move the asPercent to a common place. But if it makes sense the server to send the value already formatted then I wouldn't think it as a drawback but as an improvement. My question is does it make sense sending the value already formatted from the server? For what I've seen we never send any formatted value from the server, we always leave this responsibility to the client.

FYI: I'm not against moving the asPercent, I only want to keep consistency across all APIs we've got.

Copy link
Member

Choose a reason for hiding this comment

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

@smith: why? (Other than it being the convention)

Copy link
Contributor

Choose a reason for hiding this comment

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

On a technical level, some explanation is in #70161. If a users are using different themes and the API includes the colors of the lines the API would have to return different responses based on the theme.

If the presentational elements are coupled with the chart data, changing the presentation becomes more difficult.

Copy link
Member

Choose a reason for hiding this comment

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

For theming then it makes sense, although I can imagine that can be supported on the server as well. Personally I'm not swayed by "presentational data belongs on the client". IME, server-side code is more predictable and testable, and it's nice to have it in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, for now then I'll move the asPercent to the common directory and return the value already formatted from the server. We can continue discussing on Nathan's issue #70161.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good @cauemarcondes 👍

}))}
noHits={noHits}
/>
</EuiFlexItem>
</EuiFlexGroup>
</EuiPanel>
Expand Down
14 changes: 11 additions & 3 deletions x-pack/plugins/apm/public/components/shared/charts/CustomPlot/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class InnerCustomPlot extends PureComponent {
(series) => {
return series.slice(
0,
VISIBLE_LEGEND_COUNT + getHiddenLegendCount(series)
this.props.visibleLegendCount + getHiddenLegendCount(series)
);
}
);
Expand Down Expand Up @@ -124,14 +124,20 @@ export class InnerCustomPlot extends PureComponent {
}

render() {
const { series, truncateLegends, width, annotations } = this.props;
const {
series,
truncateLegends,
width,
annotations,
visibleLegendCount,
} = this.props;

if (!width) {
return null;
}

const hiddenSeriesCount = Math.max(
series.length - VISIBLE_LEGEND_COUNT - getHiddenLegendCount(series),
series.length - visibleLegendCount - getHiddenLegendCount(series),
0
);
const visibleSeries = this.getVisibleSeries({ series });
Expand Down Expand Up @@ -235,6 +241,7 @@ InnerCustomPlot.propTypes = {
})
),
noHits: PropTypes.bool,
visibleLegendCount: PropTypes.number,
};

InnerCustomPlot.defaultProps = {
Expand All @@ -244,6 +251,7 @@ InnerCustomPlot.defaultProps = {
truncateLegends: false,
xAxisTickSizeOuter: 0,
noHits: false,
visibleLegendCount: VISIBLE_LEGEND_COUNT,
};

export default makeWidthFlexible(InnerCustomPlot);
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/public/components/shared/charts/CustomPlot/plotUtils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const XY_HEIGHT = unit * 16;
const XY_MARGIN = {
top: unit,
left: unit * 5,
right: 0,
right: unit,
bottom: unit * 2,
};

Expand Down
Loading