[APM] Backend operation distribution chart #134561

dgieselaar · 2022-06-16T11:45:47Z

Closes #133483.

TBD:

Use sampleRangeFrom and sampleRangeTo to filter the table after [APM] Display top spans for operation #134179 is merged.

Closes elastic#133483.

…on-distribution-chart

elasticmachine · 2022-06-16T11:45:49Z

Pinging @elastic/apm-ui (Team:apm)

…on-distribution-chart

TinaHeiligers

The type change looks ok to me.

TinaHeiligers · 2022-06-16T23:17:12Z

src/core/types/elasticsearch/search.ts

@@ -536,6 +544,7 @@ export type AggregateOf<
          {
            doc_count: number;
            key: string | number;
+            key_as_string?: string;


What's the intention of the key_as_string optional type? Is it a workaround for the mixed key: string | number type declaration or for ease of use?

key_as_string is returned by Elasticsearch when the terms agg runs on a boolean (and possibly numerical) field.

…on-distribution-chart

cauemarcondes · 2022-06-22T18:05:17Z

@dgieselaar I noticed that when comparison is toggled off the sparklines are not hiding it.

Screen.Recording.2022-06-22.at.2.04.02.PM.mov

cauemarcondes

LGTM, I added some comments but they don't block it to be merged. Great work 👏🏻

cauemarcondes · 2022-06-22T18:10:19Z

x-pack/plugins/apm/public/components/app/backend_operation_detail_view/index.tsx

@@ -42,6 +43,9 @@ export function BackendOperationDetailView() {
          <BackendMetricCharts />
        </ChartPointerEventContextProvider>
      </EuiFlexItem>
+      <EuiFlexItem>
+        <BackendOperationDistributionChart />


I think to match the layout of the other charts you need to wrap this inside a <EuiPanel>

Thanks! That actually looks much better. I've also added it around the table.

cauemarcondes · 2022-06-22T18:16:04Z

...public/components/app/backend_operation_detail_view/backend_operation_distribution_chart.tsx

+      onChartSelection={selectSampleFromChartSelection}
+      onClearSelection={clearChartSelection}
+      status={status}
+      markerCurrentEvent={markerCurrentEvent}


markerCurrentEvent is a const and initialized with undefined can't you just omit this property?

Suggested change

markerCurrentEvent={markerCurrentEvent}

I can, but I prefer to be explicit about it (see line 23-24)

cauemarcondes · 2022-06-22T18:19:18Z

...public/components/app/backend_operation_detail_view/backend_operation_distribution_chart.tsx

+  const hasData =
+    (data?.allSpansDistribution.overallHistogram?.length ?? 0) > 0 ||
+    (data?.failedSpansDistribution.overallHistogram?.length ?? 0) > 0;


Isn't it the same as doing:

Suggested change

const hasData =

(data?.allSpansDistribution.overallHistogram?.length ?? 0) > 0 ||

(data?.failedSpansDistribution.overallHistogram?.length ?? 0) > 0;

const hasData =

!!data?.allSpansDistribution.overallHistogram?.length ||

!!data?.failedSpansDistribution.overallHistogram?.length

It is, but I prefer to explicitly check .length > 0 for readability reasons

cauemarcondes · 2022-06-22T18:31:40Z

x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx

+          chartData={chartData}
+          hasData={hasData}
+          percentileThresholdValue={percentileThresholdValue}
+          eventType={ProcessorEvent.span}


Shouldn't this be ProcessorEvent.transaction?

Yes, thank you! It needs it to show the correct label on the y-axis.

cauemarcondes · 2022-06-22T18:35:20Z

x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx


-export interface TransactionDistributionChartData {
+const NO_OF_TRANSACTIONS_LABEL = i18n.translate(


What does NO_OF_ stand for? 😅

Number of. I'll change it to NUMBER_OF_x

cauemarcondes · 2022-06-22T19:59:49Z

x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_histogram_range_steps.ts

+  }
+
+  const min = resp.aggregations.duration_min.value;
+  const max = resp.aggregations.duration_max.value * 2;


Maybe an explanation on why it is necessary to multiple by 2 would make it easier for the future.

I don't really know 😅 this is all code originally written by the ML team.

cauemarcondes · 2022-06-22T20:00:32Z

x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_percentiles.ts

+    }
+  );
+
+  // return early with no results if the search didn't return any documents


IMHO this comment is not necessary.

Nah, but I don't feel strong enough about it to remove it either.

cauemarcondes · 2022-06-22T20:02:24Z

x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_ranges.ts

+  const ranges = rangeSteps.reduce(
+    (p, to) => {
+      const from = p[p.length - 1].to;
+      p.push({ from, to });
+      return p;
+    },
+    [{ to: 0 }] as Array<{ from?: number; to?: number }>
+  );


Can't you do like this?

Suggested change

const ranges = rangeSteps.reduce(

(p, to) => {

const from = p[p.length - 1].to;

p.push({ from, to });

return p;

},

[{ to: 0 }] as Array<{ from?: number; to?: number }>

);

const ranges = rangeSteps.reduce<Array<{ from?: number; to?: number }>

>(

(p, to) => {

const from = p[p.length - 1].to;

p.push({ from, to });

return p;

},

[{ to: 0 }] );

I don't think either one is much better than the other, so I will just leave it as is.

cauemarcondes · 2022-06-22T20:05:47Z

...k/plugins/apm/server/routes/correlations/queries/fetch_failed_events_correlation_p_values.ts

+    const normalizedScore =
+      0.5 * Math.min(Math.max((bucket.score - 3.912) / 2.995, 0), 1) +
+      0.25 * Math.min(Math.max((bucket.score - 6.908) / 6.908, 0), 1) +
+      0.25 * Math.min(Math.max((bucket.score - 13.816) / 101.314, 0), 1);


wow it looks fancy 😎

cauemarcondes · 2022-06-22T20:14:25Z

x-pack/test/apm_api_integration/tests/correlations/failed_transactions.spec.ts

+      // expect(finalRawResponse?.failedTransactionsCorrelations?.length).to.eql(
+      //   30,
+      //   `Expected 30 identified correlations, got ${finalRawResponse?.failedTransactionsCorrelations?.length}.`
+      // );


If this is no longer necessary I think can be deleted.

I've uncommented it, good catch.

…on-distribution-chart

…-fix'

…eselaar/kibana into backend-operation-distribution-chart

…on-distribution-chart

kibana-ci · 2022-06-23T13:48:03Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1293	1296	+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	2.9MB	2.9MB	+3.7KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`apm`	84	86	+2

Total ESLint disabled count

id	before	after	diff
`apm`	98	100	+2

History

💔 Build #52729 failed a2d0ad4
💚 Build #52382 succeeded c124f01
💛 Build #52020 was flaky f7c9c24
💔 Build #52012 failed 9a7befb
💚 Build #51863 succeeded 63c6cbd

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

dgieselaar added 3 commits June 16, 2022 12:12

[APM] Backend operation distribution chart

6ed9279

Closes elastic#133483.

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

ef207e2

…on-distribution-chart

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

5039142

…on-distribution-chart

dgieselaar added release_note:enhancement Team:APM All issues that need APM UI Team support auto-backport Deprecated: Automatically backport this PR after it's merged v8.4.0 labels Jun 16, 2022

dgieselaar requested review from a team as code owners June 16, 2022 11:45

dgieselaar added 2 commits June 16, 2022 14:44

Update labels

e670e5e

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

ef2cac3

…on-distribution-chart

TinaHeiligers approved these changes Jun 16, 2022

View reviewed changes

dgieselaar added 9 commits June 19, 2022 12:40

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

dbfb5e9

…on-distribution-chart

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

70e4a5f

…on-distribution-chart

Take sampleRangeFrom/To into account for span operations table

18d6337

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

e0d05eb

…on-distribution-chart

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

1de0fbc

…on-distribution-chart

Remove console.log statements

63c6cbd

Merge branch 'main' into backend-operation-distribution-chart

9a7befb

Merge branch 'main' into backend-operation-distribution-chart

f7c9c24

Merge branch 'main' into backend-operation-distribution-chart

c124f01

cauemarcondes approved these changes Jun 22, 2022

View reviewed changes

dgieselaar and others added 6 commits June 23, 2022 10:21

Review feedback

52ffb11

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

fce1f44

…on-distribution-chart

Add event.outcome and truncated trace id to table

863a871

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

a2d0ad4

…on-distribution-chart

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

7694956

…-fix'

Update API tests

554f56b

dgieselaar added 2 commits June 23, 2022 14:46

Merge branch 'backend-operation-distribution-chart' of github.com:dgi…

63692d6

…eselaar/kibana into backend-operation-distribution-chart

Merge branch 'main' of github.com:elastic/kibana into backend-operati…

7fa752f

…on-distribution-chart

dgieselaar merged commit ec3e3b2 into elastic:main Jun 23, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jun 23, 2022

dgieselaar deleted the backend-operation-distribution-chart branch June 23, 2022 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Backend operation distribution chart #134561

[APM] Backend operation distribution chart #134561

dgieselaar commented Jun 16, 2022 •

edited

elasticmachine commented Jun 16, 2022

TinaHeiligers left a comment

TinaHeiligers Jun 16, 2022 •

edited

dgieselaar Jun 17, 2022

cauemarcondes commented Jun 22, 2022

cauemarcondes left a comment

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

cauemarcondes Jun 22, 2022

dgieselaar Jun 23, 2022

kibana-ci commented Jun 23, 2022

ESLint disabled line counts

Total ESLint disabled count


		export interface TransactionDistributionChartData {
		const NO_OF_TRANSACTIONS_LABEL = i18n.translate(

[APM] Backend operation distribution chart #134561

[APM] Backend operation distribution chart #134561

Conversation

dgieselaar commented Jun 16, 2022 • edited

elasticmachine commented Jun 16, 2022

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Jun 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cauemarcondes commented Jun 22, 2022

cauemarcondes 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 23, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

dgieselaar commented Jun 16, 2022 •

edited

TinaHeiligers Jun 16, 2022 •

edited