[APM] Transaction breakdown charts #39531

dgieselaar · 2019-06-24T19:20:39Z

Closes #36441.

To test this with the apm-integration-testing server, take the following steps:

Open the APM Server Dockerfile located at docker/apm-server/Dockerfile
Add ENV setup.template.overwrite=true below FROM ${apm_server_base_image} on line 23
Start the apm-integration-testing server with the following command: ~/dev/apm-integration-testing/scripts/compose.py start master --with-opbeans-java --opbeans-java-agent-branch=pr/588/head --force-build --no-kibana
Start kibana from source
Delete the apm-x-metric index, and wait until new data comes in
Navigate to the service overview and select opbeans-java

Definition of Done for APM UI

Flag and create issues of things that are left out
Before/after images or gif of UI changes
Release labels
Designer sign-off for UI related changes
PM approval for bigger, user-facing features

elasticmachine · 2019-06-24T20:28:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-27T11:21:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-27T12:01:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

dgieselaar

I've bailed on refactoring CustomPlot and its sibilings, I ran into some issues w/ react-vis not having typings and it generally just being too much work to get through the process on such short notice. Would definitely like to pick it up some time soon, feel like we can gain a lot by improving our coverage there.

dgieselaar · 2019-06-28T16:06:52Z

x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/StaticPlot.js

@@ -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}


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.

dgieselaar · 2019-06-28T16:08:53Z

x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/index.js

@@ -151,6 +191,12 @@ export class InnerCustomPlot extends PureComponent {
            tickFormatX={this.props.tickFormatX}
          />

+          {stacked
+            ? React.cloneElement(staticPlot, {


AFAICT, there's only one way to achieve the look that we want (stroke on just one side of the area), and that is by drawing another line chart on top of it. Seems overkill, but it's the recommendation from the docs:

To create a chart that has a fill, no distinct lines to the left, right or bottom, but a different line style at the top, you may create an area chart with a line chart on top.

that's unfortunate, but tolerable as long as there's not any noticeable drag to page performance.

@dgieselaar Doesn't this duplicate the the XAxis, YAxis and HorizontalGridLines? What about just adding an additional LineSeries to the chart?

dgieselaar · 2019-06-28T16:09:35Z

x-pack/legacy/plugins/apm/public/context/ChartsTimeContext.tsx

+  setTime: (time: number | null) => unknown;
+} | null>(null);
+
+const ChartsTimeContextProvider: React.FC = ({ children }) => {


Simply syncs time across all charts in its context.

This sounds similar to
https://github.com/elastic/kibana/blob/7056f6143d7863c2b48ff22ee376402c932bbb60/x-pack/legacy/plugins/apm/public/components/shared/charts/SyncChartGroup/index.tsx

Can those to be combined or do they serve different purposes?

dgieselaar · 2019-06-28T16:11:04Z

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

@@ -5,7 +5,10 @@
 */

 import { getTransactionBreakdown } from '.';
+// using .ts files results in OOM during typecheck :)


maybe we can use .json files here too?

Changed to .json

dgieselaar · 2019-06-28T16:11:47Z

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


-  return breakdowns;
+  const filteredTimeseriesPerSubtype = pick(


Ensures timeseries that are not in the top 20 KPIs are filtered out.

elasticmachine · 2019-06-28T17:43:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

formgeist · 2019-07-01T12:53:13Z

@dgieselaar A few UI problems I've found in relation to the chart.

It seems that the lines are pushed to the left (outside the x-axis area) every now and then. I'm running 15 minutes date range here, not sure how it might be related.

Can we add some space / margin on those labels or value text to split them a little apart in this list?

elasticmachine · 2019-07-01T13:44:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

dgieselaar · 2019-07-01T19:49:31Z

@formgeist both should be fixed now, thanks for catching this!

elasticmachine · 2019-07-01T20:56:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ogupte · 2019-07-02T01:40:05Z

probably not a blocker, but i noticed a weird visual bug:

ogupte · 2019-07-02T04:07:50Z

x-pack/legacy/plugins/apm/typings/timeseries.ts

@@ -14,5 +14,17 @@ export interface RectCoordinate {
  x0: number;
 }

+export interface TimeSerie {


is this spelling of TimeSerie intentional or should it be TimeSeries?

fixed, changed to TimeSeries

ogupte · 2019-07-02T04:10:41Z

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

                  value: serviceName
                }
              }
            },
            {
              term: {
-                'transaction.type.keyword': {
+                'transaction.type': {


constants for 'transaction.type' and 'transaction.name'

changed all of the field names to constants from common/elasticsearch_fieldnames.ts

elasticmachine · 2019-07-02T12:21:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

x-pack/legacy/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx

sorenlouv · 2019-07-02T13:23:05Z

...plugins/apm/public/components/shared/charts/TransactionCharts/TransactionLineChart/index.tsx

+  const noHits = series.every(
+    serie =>
+      serie.data.filter(value => 'y' in value && value.y !== null).length === 0
+  );


In the rest of the code base noHits refers to whether Elasticsearch actually returned any hits. We cannot just look at series.data because we might populate that with empty state data so the graph draws the x and y-axis.

Therefore other chart endpoints return the series.totalHits (coming from ES hits.total):

kibana/x-pack/legacy/plugins/apm/server/lib/metrics/transform_metrics_chart.ts

Line 58 in 7056f61

totalHits: hits.total,

I think we should do the same here if that makes sense to you?

It would be more consistent, but I have some doubts about using hits.total to determine whether there is no data. Gut tells me there's a lot of cases where that will be a false positive. Using the actual data passed to the charts to determine if there are any data points by filtering out all non-null values feels like a better heuristic to me. Does that make sense?

Gut tells me there's a lot of cases where that will be a false positive. Using the actual data passed to the charts to determine if there are any data points by filtering out all non-null values feels like a better heuristic to me. Does that make sense?

That does make sense. My originally assumption was that if there are any hits then the aggregation will contain (timeseries) data, and it's been true for the aggs we do for the transaction charts. But this doesn't hold true for things like derivative aggs which requires at least two data points. So using hits.total as an indicator for data might not be a good idea in general.

If we go the direction you suggest (with a custom check) we should get rid of the noHits, right? Eg:

https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/apm/public/selectors/chartSelectors.ts#L55

https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/apm/public/components/app/ServiceDetails/MetricsChart.tsx#L37

I think it might make sense to go over how we handle this across the different visualizations. It seems like a pretty standard thing for a chart to be able to know whether it has data or not, and would be great if we can stick to one approach.

...plugins/apm/public/components/shared/charts/TransactionCharts/TransactionLineChart/index.tsx

Closes elastic#36441.

…ent unnecessary re-renders

elasticmachine · 2019-07-02T15:13:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* [APM] Transaction breakdown graphs Closes elastic#36441. * Don't use .keyword now that mapping is correct * Use better heuristic for determining noHits; wrap App in memo to prevent unnecessary re-renders * Remove nested panel around TransactionBreakdown * Fix display issues in charts * Address review feedback * Address additional review feedback

* [APM] Transaction breakdown graphs Closes #36441. * Don't use .keyword now that mapping is correct * Use better heuristic for determining noHits; wrap App in memo to prevent unnecessary re-renders * Remove nested panel around TransactionBreakdown * Fix display issues in charts * Address review feedback * Address additional review feedback

sorenlouv · 2019-07-02T22:14:20Z

x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/plotUtils.js

-  const xMax = d3.max(flattenedCoordinates, d => d.x);
+  const xMin = start ? start : d3.min(flattenedCoordinates, d => d.x);
+
+  const xMax = end ? end : d3.max(flattenedCoordinates, d => d.x);


Shouldn't start be called xMin and end be called xMax?

sorenlouv · 2019-07-02T22:19:34Z

x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/plotUtils.js

-    XY_WIDTH: width
+    XY_HEIGHT: height || XY_HEIGHT,
+    XY_WIDTH: width,
+    ...(stacked ? { stackBy: 'y' } : {})


Does this need to be conditional? Doesn't it only take effect when things are stacked? So setting it to stackBy: 'y' only affects stacked charts.

sorenlouv · 2019-07-02T22:21:10Z

x-pack/legacy/plugins/apm/public/components/shared/charts/CustomPlot/plotUtils.js

  };
 }

 export function SharedPlot({ plotValues, ...props }) {
+  const { XY_HEIGHT: height, XY_MARGIN: margin, XY_WIDTH: width } = plotValues;


I'm generally not a fan of renaming thing like this. If the names are not good enough, why not fix them at the root? Instead we end up with multiple names for the same thing.

sorenlouv · 2019-07-02T22:26:56Z

...lugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownGraph/index.tsx

+    () => {
+      return timeseries.map(timeseriesConfig => {
+        return {
+          title: timeseriesConfig.name,


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.

sorenlouv · 2019-07-02T22:32:23Z

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

+      }
+    });
+
+    const { timeseries_per_subtype: timeseriesPerSubtype } = response;


Why are we using snake case in the first place? All APM endpoints return camel cased data currently.

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

Line 191 in 868c712

timeseries_per_subtype: timeseriesPerSubtype

sorenlouv · 2019-07-02T22:42:16Z

x-pack/legacy/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx

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


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

sorenlouv · 2019-07-02T22:51:03Z

x-pack/legacy/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx


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

-  const kpis = useMemo(
+  const kpis = data ? data.kpis : undefined;
+  const timeseriesPerSubtype = data ? data.timeseries_per_subtype : undefined;


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 }

sorenlouv · 2019-07-03T10:05:51Z

...lugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownGraph/index.tsx

+  timeseries: Array<{
+    name: string;
+    color: string;
+    values: Array<{ x: number; y: number | null }>;


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?

sorenlouv · 2019-07-03T10:11:37Z

...plugins/apm/public/components/shared/charts/TransactionCharts/TransactionLineChart/index.tsx

+    title: React.ReactNode;
+    titleShort?: React.ReactNode;
+    data: Array<Coordinate | RectCoordinate>;
+    type: string;


Isn't this a duplicate of

kibana/x-pack/legacy/plugins/apm/typings/timeseries.ts

Lines 17 to 27 in 1e1153e

export interface TimeSeries {

title: string;

titleShort?: string;

hideLegend?: boolean;

hideTooltipValue?: boolean;

data: Array<Coordinate | RectCoordinate>;

legendValue?: string;

type: string;

color: string;

areaColor?: string;

}

?

Or rather: can they be combined?

dgieselaar force-pushed the transaction-breakdown-graph branch from 26c86e1 to 9bef809 Compare June 24, 2019 19:23

dgieselaar force-pushed the transaction-breakdown-graph branch 3 times, most recently from ca232ba to d27fe32 Compare June 27, 2019 10:19

dgieselaar force-pushed the transaction-breakdown-graph branch 2 times, most recently from 1fe4eaf to 85985c0 Compare June 27, 2019 20:01

dgieselaar mentioned this pull request Jun 28, 2019

[APM] Transactions breakdown graph: Add breakdown KPI component #38658

Merged

5 tasks

dgieselaar force-pushed the transaction-breakdown-graph branch from 85985c0 to 7b6608f Compare June 28, 2019 16:04

dgieselaar commented Jun 28, 2019

View reviewed changes

dgieselaar changed the title ~~Transaction breakdown graph~~ [APM] Transaction breakdown graph Jun 28, 2019

dgieselaar changed the title ~~[APM] Transaction breakdown graph~~ [APM] Transaction breakdown charts Jun 28, 2019

dgieselaar requested review from formgeist and ogupte June 28, 2019 16:13

dgieselaar marked this pull request as ready for review June 28, 2019 16:16

dgieselaar requested a review from a team as a code owner June 28, 2019 16:16

dgieselaar added the release_note:roadmap label Jun 28, 2019

dgieselaar force-pushed the transaction-breakdown-graph branch from 7b6608f to 06b9ea7 Compare July 1, 2019 11:29

dgieselaar force-pushed the transaction-breakdown-graph branch from e5befc2 to e59ec19 Compare July 1, 2019 19:48

dgieselaar force-pushed the transaction-breakdown-graph branch from e59ec19 to bd659c0 Compare July 1, 2019 19:51

ogupte reviewed Jul 2, 2019

View reviewed changes

dgieselaar force-pushed the transaction-breakdown-graph branch from bd659c0 to 556c3a6 Compare July 2, 2019 11:21

sorenlouv reviewed Jul 2, 2019

View reviewed changes

x-pack/legacy/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Jul 2, 2019

View reviewed changes

...plugins/apm/public/components/shared/charts/TransactionCharts/TransactionLineChart/index.tsx Outdated Show resolved Hide resolved

dgieselaar added 6 commits July 2, 2019 16:03

[APM] Transaction breakdown graphs

2f2534f

Closes elastic#36441.

Don't use .keyword now that mapping is correct

a4149c6

Use better heuristic for determining noHits; wrap App in memo to prev…

49afb52

…ent unnecessary re-renders

Remove nested panel around TransactionBreakdown

52945d0

Fix display issues in charts

a7e701a

Address review feedback

d72a3c6

dgieselaar force-pushed the transaction-breakdown-graph branch 2 times, most recently from 56865ed to 714ec19 Compare July 2, 2019 14:06

Address additional review feedback

c808e71

dgieselaar force-pushed the transaction-breakdown-graph branch from 714ec19 to c808e71 Compare July 2, 2019 14:07

dgieselaar merged commit 73bb21b into elastic:master Jul 2, 2019

dgieselaar deleted the transaction-breakdown-graph branch July 2, 2019 15:14

dgieselaar mentioned this pull request Jul 2, 2019

[7.x] [APM] Transaction breakdown charts (#39531) #40156

Merged

sorenlouv reviewed Jul 2, 2019

View reviewed changes

sorenlouv reviewed Jul 3, 2019

View reviewed changes


		return breakdowns;
		const filteredTimeseriesPerSubtype = pick(

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

	export interface TimeSeries {
	title: string;
	titleShort?: string;
	hideLegend?: boolean;
	hideTooltipValue?: boolean;
	data: Array<Coordinate \| RectCoordinate>;
	legendValue?: string;
	type: string;
	color: string;
	areaColor?: string;
	}

[APM] Transaction breakdown charts #39531

[APM] Transaction breakdown charts #39531

Conversation

dgieselaar commented Jun 24, 2019 • edited Loading

Definition of Done for APM UI

elasticmachine commented Jun 24, 2019

💔 Build Failed

elasticmachine commented Jun 27, 2019

💔 Build Failed

elasticmachine commented Jun 27, 2019

💚 Build Succeeded

dgieselaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 28, 2019

💔 Build Failed

formgeist commented Jul 1, 2019

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

dgieselaar commented Jul 1, 2019

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

ogupte commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 2, 2019

💚 Build Succeeded

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 2, 2019

💚 Build Succeeded

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Jun 24, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading

sorenlouv Jul 2, 2019 •

edited

Loading