[APM] Optimize traces overview #70200

dgieselaar · 2020-06-29T14:10:24Z

Similar to #69648, achieves a 30%-50% performance improvement of the traces overview. Could possibly improve that even more by not loading a sample, as that seems to be the slowest request of the bunch, and is only used for deeplinking, so could be lazily-loaded.

Of note:

removed transform.ts and apply the transformation logic in/after fetching.
removed client-side logic for calculating relative impact. FWICT this was already happening on the server.

elasticmachine · 2020-06-29T14:10:28Z

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

…es-overview

dgieselaar · 2020-06-29T20:57:03Z

I've made some changes in aggregations.ts to support union types in aggregations. E.g., we can now support { transaction_groups: { composite: any } | { terms: any } }. This also allows us to clean up code in a couple of other places.

sorenlouv · 2020-06-29T21:04:43Z

x-pack/plugins/apm/server/lib/transaction_groups/get_metrics.ts

+  return arrayUnionToCallable(
+    response.aggregations?.transaction_groups.buckets
+  ).map((bucket) => {


Is this the equivalent of?

Suggested change

return arrayUnionToCallable(

response.aggregations?.transaction_groups.buckets

).map((bucket) => {

return (response.aggregations?.transaction_groups.buckets ?? []).map((bucket) => {

no, it's this issue: https://ela.st/union-callable

sorenlouv · 2020-06-29T21:07:36Z

x-pack/plugins/apm/public/hooks/useTransactionList.ts

  return {
-    data: memoizedData,
+    data,
    status,
    error,
  };


Thanks for cleaning up 👍

sorenlouv · 2020-06-29T21:08:10Z

x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_alert_type.ts

@@ -157,7 +157,7 @@ export function registerTransactionDurationAlertType({

      const { agg } = response.aggregations;

-      const value = 'values' in agg ? agg.values[0] : agg?.value;
+      const value = 'values' in agg ? Object.values(agg.values)[0] : agg?.value;


is this an unrelated change?

Actually, this is a bug fix now that I think of it. This code (written by me of course) assumes that the response of a percentiles aggregation is number[], but it's actually Record<string, number> where the key is the percentile. I'll verify whether that is the case and file a separate bug for it.

sorenlouv · 2020-06-29T21:12:45Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

      aggs: {
        transaction_groups: {
-          composite: {
-            size: bucketSize + 1, // 1 extra bucket is added to check whether the total number of buckets exceed the specified bucket size.


We should retain the comment to explain the + 1

there's a comment lower in the file as well, do you think we need both? (no strong opinions from my side)

re-added the comment

sorenlouv · 2020-06-29T21:13:22Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

+                  field: TRANSACTION_NAME,
+                  size,
+                },
+              }),


Any reason not to keep it as a composite agg?

Yeah: performance, and a terms aggregation is sorted by doc_count. Sorting on composite aggregations is unpredictable, so there's a higher risk of transaction groups with lots of transactions dropping off the list.

sorenlouv · 2020-06-29T21:16:11Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

+  allMetrics.forEach((metric) => {
+    // we use indexOf so we can replace the existing item with a new one
+    // this will give us type safety (Object.assign is unsafe)
+    let indexOf = findIndex(items, (i) => isEqual(i.key, metric.key));
+    let item = items[indexOf];
+
+    if (indexOf === -1) {
+      const newItem = {
+        key: metric.key,
+      };
+      items.push(newItem);
+      item = newItem;
+      indexOf = items.length - 1;
+    }
+
+    const newItem = {
+      ...item,
+      ...metric,
+    };
+
+    items[indexOf] = newItem;
+  });


This looks pretty intimidating. Can we extract this logic and add some comments?
Is the new logic or what does it replace?

I hate it too! I had Object.assign({}) first which was cleaner, but I ran into a bug because there was no type checking of the values I was assigning.

Comments might clear it up but I'm open to suggestions as well. This code is trying to flatten buckets from multiple requests by either a grouping record (service.name and transaction.name) or a grouping key (transaction.name).

sorenlouv · 2020-06-29T21:20:23Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

+  const max = Math.max(...values);
+  const min = Math.min(...values);
+
+  const duration = moment.duration(setup.end - setup.start);
+  const minutes = duration.asMinutes();
+
+  const itemsWithRelativeImpact: TransactionGroup[] = items
+    .map((item) => {
+      return {
+        key: item.key,
+        averageResponseTime: item.avg,
+        transactionsPerMinute: (item.count ?? 0) / minutes,
+        impact:
+          item.sum !== null && item.sum !== undefined
+            ? ((item.sum - min) / (max - min)) * 100 || 0
+            : 0,
+        p95: item.p95,
+        sample: item.sample!,
+      };
+    })
+    .filter((item) => item.sample);


Any reason not having this in a separate function?

sorenlouv · 2020-06-29T23:35:34Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts


-  const params = mergeProjection(projection, {
+  const request = mergeProjection(projection, {


I keep running into the same issue: when I try to review these changes I end up in the rabbit hole of projections.
I started flattening it back into a single file (mostly) and I think it really improves the readability and makes it easier to spot problems.

I think it might be a good idea to go back to the drawing board and find a way to simplify ui filters (and thus projections).

My concern is that we're forgetting why we have projections, and end up duplicating large parts of our queries in separate files. Tbh, I don't think projections are the cause of - or the solution to - some of our issues. But curious to see what you come up with.

I don't think projections are the cause of - or the solution to - some of our issues

Projections most definitely make it harder for me to make sense of the queries. But I'm not sure my suggestions are any better (I obviously think they are but I also know that most things sound better before they are converted to code).

…es-overview

dgieselaar · 2020-07-06T08:09:16Z

@elasticmachine merge upstream

dgieselaar · 2020-07-06T13:20:36Z

@elasticmachine merge upstream

dgieselaar · 2020-07-08T14:35:09Z

@elasticmachine merge upstream

…es-overview

…na into optimize-traces-overview

smith

I didn't notice a significant performance improvement, but the code looks good.

smith · 2020-07-27T20:57:10Z

x-pack/plugins/apm/server/lib/metrics/fetch_and_transform_metrics.ts

+      aggs: {
+        timeseriesData: {
+          date_histogram: AggregationOptionsByType['date_histogram'];
+          aggs: Record<string, MetricsAggregationMap>;


Could this just be:

Suggested change

aggs: Record<string, MetricsAggregationMap>;

aggs: Aggs;

yep! Changed it to MetricAggs in all places.

smith · 2020-07-27T20:57:32Z

x-pack/plugins/apm/server/lib/metrics/fetch_and_transform_metrics.ts

+          date_histogram: AggregationOptionsByType['date_histogram'];
+          aggs: Record<string, MetricsAggregationMap>;
+        };
+      } & Record<string, MetricsAggregationMap>;


Could this just be:

Suggested change

} & Record<string, MetricsAggregationMap>;

} & Aggs;

ogupte · 2020-07-27T22:47:13Z

x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

+        sample: item.sample!,
+      };
+    })
+    .filter((item) => item.sample);


should the filter for itemsWithRelativeImpact be .filter((item) => item.impact > 0) or is that logically equivalent to whether or not there is a sample?

I think it's pretty much the same, but the filter is mostly a safeguard for ensuring there's actually a sample as we can't deeplink without it. Previously a sample was guaranteed because it's all one request, but splitting it up into smaller requests means that is no longer the case - for instance, when there are more than 10k buckets.

ogupte · 2020-07-27T22:54:22Z

x-pack/plugins/apm/public/components/app/TraceOverview/TraceList.tsx

-      <EuiToolTip id="trace-transaction-link-tooltip" content={name}>
+    render: (_: string, { sample }: TransactionGroup) => (
+      <EuiToolTip
+        id="trace-transaction-link-tooltip"


I know this line was just name changes, but still: I don't think the id trace-transaction-link-tooltip is necessary anymore. I don't see it referenced anywhere, and i don't think it's used in any unit/functional testing.

…es-overview

kibanamachine · 2020-07-28T13:24:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a86551b

Build metrics

async chunks size

id	value	diff	baseline
apm	3.7MB	-3.2KB	3.7MB

History

💚 Build #64299 succeeded 861cf63
💚 Build #58944 succeeded 6bf8dbb
💔 Build #58830 failed 804df26
💔 Build #58780 failed 5126db9
💚 Build #58647 succeeded dff5fdc

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-07-28T13:53:53Z

💚 Backport successful

The PR was backported to the following branches:

✅ 7.9

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

dgieselaar added 2 commits June 29, 2020 14:37

[APM] Optimize traces overview

2b1f4c3

Separate queries into separate file

e7ac578

dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 29, 2020

dgieselaar requested a review from a team as a code owner June 29, 2020 14:10

botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 29, 2020

dgieselaar added 2 commits June 29, 2020 22:50

Support union types for aggregations

bfac5c2

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

43bec06

…es-overview

sorenlouv reviewed Jun 29, 2020

View reviewed changes

dgieselaar added 5 commits June 30, 2020 09:08

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

fcf18b2

…es-overview

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

fd73424

…es-overview

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

f80ff38

…es-overview

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

0395763

…es-overview

Separate function for calculating relative impact

dff5fdc

dgieselaar mentioned this pull request Jul 3, 2020

[APM] Use apmEventClient for querying APM event indices dgieselaar/kibana#3

Closed

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

5126db9

…es-overview

Merge branch 'master' into optimize-traces-overview

804df26

dgieselaar mentioned this pull request Jul 6, 2020

[APM] Percentiles-based transaction duration alerts never fire #70770

Closed

Merge branch 'master' into optimize-traces-overview

6bf8dbb

elasticmachine and others added 7 commits July 8, 2020 08:35

Merge branch 'master' into optimize-traces-overview

0bee339

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

381304f

…es-overview

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

a13cfc6

…es-overview

Update traces functional test

9fb2d12

Update API tests

ccf9654

Update responses for functional tests

b040a40

Merge branch 'optimize-traces-overview' of github.com:dgieselaar/kiba…

861cf63

…na into optimize-traces-overview

dgieselaar requested a review from a team July 27, 2020 17:59

smith approved these changes Jul 27, 2020

View reviewed changes

ogupte reviewed Jul 27, 2020

View reviewed changes

dgieselaar added 2 commits July 28, 2020 09:29

Merge branch 'master' of github.com:elastic/kibana into optimize-trac…

72eb667

…es-overview

Review feedback

a86551b

dgieselaar merged commit 19532fc into elastic:master Jul 28, 2020

kibanamachine pushed a commit that referenced this pull request Jul 28, 2020

[APM] Optimize traces overview (#70200)

e42e671

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine mentioned this pull request Jul 28, 2020

[7.9] [APM] Optimize traces overview (#70200) #73444

Closed

dgieselaar added v7.10.0 and removed v7.9.0 labels Jul 28, 2020

dgieselaar mentioned this pull request Jul 28, 2020

[7.x] [APM] Optimize traces overview (#70200) #73445

Merged

dgieselaar added a commit that referenced this pull request Jul 29, 2020

[7.x] [APM] Optimize traces overview (#70200) (#73445)

bb2e167

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Optimize traces overview #70200

[APM] Optimize traces overview #70200

dgieselaar commented Jun 29, 2020 •

edited

Loading

elasticmachine commented Jun 29, 2020

dgieselaar commented Jun 29, 2020

sorenlouv Jun 29, 2020

dgieselaar Jun 29, 2020

sorenlouv Jun 29, 2020

sorenlouv Jun 29, 2020

dgieselaar Jun 29, 2020

dgieselaar Jul 6, 2020

sorenlouv Jun 29, 2020

dgieselaar Jun 29, 2020

dgieselaar Jul 3, 2020

sorenlouv Jun 29, 2020

dgieselaar Jun 29, 2020 •

edited

Loading

sorenlouv Jun 29, 2020

dgieselaar Jun 29, 2020

sorenlouv Jun 29, 2020

sorenlouv Jun 29, 2020

dgieselaar Jun 30, 2020

sorenlouv Jul 1, 2020 •

edited

Loading

dgieselaar commented Jul 6, 2020

dgieselaar commented Jul 6, 2020

dgieselaar commented Jul 8, 2020

smith left a comment

smith Jul 27, 2020

dgieselaar Jul 28, 2020

smith Jul 27, 2020

ogupte Jul 27, 2020

dgieselaar Jul 28, 2020

ogupte Jul 27, 2020 •

edited

Loading

dgieselaar Jul 28, 2020

kibanamachine commented Jul 28, 2020

kibanamachine commented Jul 28, 2020


		const params = mergeProjection(projection, {
		const request = mergeProjection(projection, {

[APM] Optimize traces overview #70200

[APM] Optimize traces overview #70200

Conversation

dgieselaar commented Jun 29, 2020 • edited Loading

elasticmachine commented Jun 29, 2020

dgieselaar commented Jun 29, 2020

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

dgieselaar Jun 29, 2020 • 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

Choose a reason for hiding this comment

sorenlouv Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

dgieselaar commented Jul 6, 2020

dgieselaar commented Jul 6, 2020

dgieselaar commented Jul 8, 2020

smith 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

ogupte Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jul 28, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

kibanamachine commented Jul 28, 2020

💚 Backport successful

dgieselaar commented Jun 29, 2020 •

edited

Loading

dgieselaar Jun 29, 2020 •

edited

Loading

sorenlouv Jul 1, 2020 •

edited

Loading

ogupte Jul 27, 2020 •

edited

Loading