[APM]: Inferred types for aggregations #37360

dgieselaar · 2019-05-29T17:04:34Z

Previously, aggregations returned by the ESClient were 'any' by default, and the return type had to be explicitly defined by the consumer to get any type safety. This leads to both type duplication and errors because of wrong assumptions.

This change infers the aggregation return type from the parameters passed to ESClient.search.

e.g. from the following parameters:

const params = {
  body: {
    aggs: {
      by_interval: {
        date_histogram: {
          field: '@timestamp'
        },
        aggs: {
          by_service_name: {
            terms: {
              field: 'service_name.keyword'
            }
          }
        }
      }
    }
  }
};

it will infer the return type (approximately):

const response = {
  aggregations: {
    by_interval: {
      buckets: Array<{
        doc_count: number;
        key: number;
        key_as_string: string;
        by_service_name: {
          buckets: Array<{
            doc_count:number;
            key: string;
            key_as_string:string;
          }>
        }
      }>
    }
  }
}

Here's a TypeScript REPL.

dgieselaar · 2019-05-29T17:05:16Z

x-pack/plugins/apm/server/lib/errors/get_error_groups.ts

@@ -37,7 +36,10 @@ export async function getErrorGroups({
 }) {
  const { start, end, uiFiltersES, client, config } = setup;

-  const params: SearchParams = {
+  // sort buckets by last occurrence of error


TS was complaining about changing the shape of the object after creating it, so I moved it inline.

dgieselaar · 2019-05-29T17:05:33Z

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

@@ -22,14 +22,20 @@ const getRelativeImpact = (
  );

 function getWithRelativeImpact(items: TransactionListAPIResponse) {
-  const impacts = items.map(({ impact }) => impact);
+  const impacts = items


values from metric aggregations can be null too

dgieselaar · 2019-05-29T17:08:30Z

x-pack/plugins/apm/server/lib/metrics/by_agent/java/heap_memory/index.ts

@@ -52,6 +50,9 @@ const chartBase: ChartBase<HeapMemoryMetrics> = {
 };

 export async function getHeapMemoryChart(setup: Setup, serviceName: string) {
-  const result = await fetch(setup, serviceName);
-  return transformDataToMetricsChart<HeapMemoryMetrics>(result, chartBase);
+  const result = (await fetch(setup, serviceName)) as MetricSearchResponse<


If we don't explicitly tell which keys from aggregations are related to the chart, TS will create a union type of date_histogram and min/max buckets, and TS does not allow (AFAICT) maps over union types: microsoft/TypeScript#29011 (comment)

dgieselaar · 2019-05-29T17:09:58Z

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

+  CpuSearchResponse,
+  'aggregations'
+> & {
+  aggregations: IndexAsString<


Sometimes TS needs a little help in realising object keys are strings (something to do with the keyof operator).

i can understand it...since numbers and symbols are valid object keys too, but in our case, it will always be strings :)

dgieselaar · 2019-05-29T17:10:42Z

x-pack/plugins/apm/server/lib/transactions/charts/get_anomaly_data/fetcher.ts

@@ -91,7 +74,8 @@ export async function anomalySeriesFetcher({
  };

  try {
-    return await client<void, Aggs>('search', params);
+    const response = await client<void, typeof params>('search', params);
+    return response;


it's kind of weird to me that this is in a try/catch and silently fails when there is a HttpError. Anyone who knows why?

i don't know why it's like this, but i agree it seems strange. I would imagine that it might be this way so that a chart is simply omitted from rendering when there is an error, rather than displaying a chart + error messaging. But it seems like this is incomplete UX.

dgieselaar · 2019-05-29T17:13:14Z

x-pack/plugins/apm/typings/elasticsearch.ts

+            lower: number;
+          };
+        };
+      }[AggregationType & keyof AggregationOption[AggregationName]]


AggregationOption:

{ name_of_agg: { terms: { ... } } }

That needs to be translated to:

{ name_of_agg: { buckets: [] } }

That's what this part is for. It picks whatever key from name_of_agg that is an aggregation type, and uses that to get the response type from the parent object. Dirty, but works.

elasticmachine · 2019-05-29T20:09:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-31T13:28:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-11T17:33:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-12T12:35:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-12T13:13:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-13T14:22:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ogupte

This is really great stuff. takes out much of the boilerplate type definitions, and fixes inconsistencies with ES types. Just a few comments I left below.

ogupte · 2019-06-13T18:46:33Z

x-pack/plugins/apm/server/lib/transactions/charts/get_anomaly_data/fetcher.ts

@@ -91,7 +74,8 @@ export async function anomalySeriesFetcher({
  };

  try {
-    return await client<void, Aggs>('search', params);
+    const response = await client<void, typeof params>('search', params);
+    return response;


i don't know why it's like this, but i agree it seems strange. I would imagine that it might be this way so that a chart is simply omitted from rendering when there is an error, rather than displaying a chart + error messaging. But it seems like this is incomplete UX.

ogupte · 2019-06-13T19:16:31Z

x-pack/plugins/apm/server/lib/metrics/by_agent/java/heap_memory/index.ts

+    keyof typeof chartBase.series
+  >;
+
+  return transformDataToMetricsChart(result, chartBase as ChartBase);


These type assertions could be obscuring type errors in the value. For chartBase, i like how we no longer have to define a special interface for chart-specific metrics. but it's now possible that a type mismatch for the rest of chartBase makes it thru to transformDataToMetricsChart via the type assertion. It might be safer for chartBase to be typed when it's defined.

A possible solution for best of both worlds:

const chartBaseSeries = { heapMemoryUsed: { title: i18n.translate('xpack.apm.agentMetrics.java.heapMemorySeriesUsed', { defaultMessage: 'Avg. used' }), color: theme.euiColorVis0 }, heapMemoryCommitted: { title: i18n.translate( 'xpack.apm.agentMetrics.java.heapMemorySeriesCommitted', { defaultMessage: 'Avg. committed' } ), color: theme.euiColorVis1 }, heapMemoryMax: { title: i18n.translate('xpack.apm.agentMetrics.java.heapMemorySeriesMax', { defaultMessage: 'Avg. limit' }), color: theme.euiColorVis2 } }; const chartBase: ChartBase = { title: i18n.translate('xpack.apm.agentMetrics.java.heapMemoryChartTitle', { defaultMessage: 'Heap Memory' }), key: 'heap_memory_area_chart', type: 'area', yUnit: 'bytes', series: chartBaseSeries }; export async function getHeapMemoryChart(setup: Setup, serviceName: string) { const result = await fetch<keyof typeof chartBaseSeries>(setup, serviceName); return transformDataToMetricsChart(result, chartBase); }

with fetch defined as type:

export async function fetch<ChartBaseSeriesKey extends string>( setup: Setup, serviceName: string ): Promise<MetricSearchResponse<ChartBaseSeriesKey>> { ...

Yeah definitely looks better! I tried to make this work but ran into some issues when casting the return value to the return type. I've landed on a solution where instead of having several fetchers and fetch calls, we have a generic fetchAndTransformMetrics call with some parameters. fetchAndTransformMetrics then calls transformDataToMetricsChart, and TypeScript then - for some reason - is able to match the types together. Bonus is that we can merge a bunch of largely overlapping fetcher files into one file.

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

ogupte · 2019-06-13T19:54:12Z

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

+  CpuSearchResponse,
+  'aggregations'
+> & {
+  aggregations: IndexAsString<


i can understand it...since numbers and symbols are valid object keys too, but in our case, it will always be strings :)

ogupte · 2019-06-13T19:56:27Z

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

-  const values = transactionGroups.map(({ impact }) => impact);
+  const values = transactionGroups
+    .map(({ impact }) => impact)
+    .filter(value => value !== null) as number[];


the type assertion as number[] is not needed here since it's inferred correctly

impact has a union type of number and null, and TypeScript can't figure out that we filtered out all non-null values in the filter function before it. I think in Lodash 4.x you can use compact to achieve the same effect, but the typings for 3.x are not complete enough.

ogupte · 2019-06-13T19:57:40Z

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

@@ -27,7 +34,7 @@ function getTransactionGroup(
  const averageResponseTime = bucket.avg.value;
  const transactionsPerMinute = bucket.doc_count / minutes;
  const impact = bucket.sum.value;
-  const sample = bucket.sample.hits.hits[0]._source;
+  const sample = bucket.sample.hits.hits[0]._source as Transaction;


this type assertion as Transaction should not be needed here since it inferred from _source

That works for the top-level hits object, but this is from a top_hits aggregation, so it needs a type hint. I'm not sure how to solve it without the type hint because it's so dynamic. Do you have any ideas?

in this case, i think you're right to use the type assertion

elasticmachine · 2019-06-15T21:49:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-16T11:38:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

dgieselaar · 2019-06-16T11:41:50Z

retest

elasticmachine · 2019-06-16T12:38:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Previously, aggregations returned by the ESClient were 'any' by default, and the return type had to be explicitly defined by the consumer to get any type safety. This leads to both type duplication and errors because of wrong assumptions. This change infers the aggregation return type from the parameters passed to ESClient.search.

… tests

elasticmachine · 2019-06-17T08:10:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ogupte

Looks great!

* [APM]: Inferred types for aggregations Previously, aggregations returned by the ESClient were 'any' by default, and the return type had to be explicitly defined by the consumer to get any type safety. This leads to both type duplication and errors because of wrong assumptions. This change infers the aggregation return type from the parameters passed to ESClient.search. * Fix idx error * Safeguard against querying against non-existing indices in functional tests * Improve metric typings * Automatically infer params from function arguments * Remove unnecessary type hints

dgieselaar requested a review from a team as a code owner May 29, 2019 17:04

dgieselaar commented May 29, 2019

View reviewed changes

dgieselaar requested review from ogupte and sorenlouv May 31, 2019 07:11

dgieselaar force-pushed the aggregation-types branch from 142e695 to d0cf61f Compare May 31, 2019 10:26

dgieselaar force-pushed the aggregation-types branch from d0cf61f to d1c1a77 Compare June 11, 2019 14:31

dgieselaar mentioned this pull request Jun 11, 2019

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

Merged

5 tasks

dgieselaar force-pushed the aggregation-types branch from d1c1a77 to 1d78e90 Compare June 12, 2019 09:33

dgieselaar force-pushed the aggregation-types branch 2 times, most recently from 50c1548 to a5d4f1d Compare June 13, 2019 13:24

dgieselaar added the release_note:skip Skip the PR/issue when compiling release notes label Jun 13, 2019

ogupte reviewed Jun 13, 2019

View reviewed changes

dgieselaar force-pushed the aggregation-types branch from a5d4f1d to 0181fe2 Compare June 15, 2019 20:50

dgieselaar force-pushed the aggregation-types branch from 2f055e3 to dc0da4b Compare June 16, 2019 10:44

dgieselaar added 6 commits June 17, 2019 09:13

Fix idx error

bd4bf48

Safeguard against querying against non-existing indices in functional…

d6b0d1e

… tests

Improve metric typings

676a69f

Automatically infer params from function arguments

dc9e936

Remove unnecessary type hints

8ac37e6

dgieselaar force-pushed the aggregation-types branch from dc0da4b to 8ac37e6 Compare June 17, 2019 07:14

dgieselaar requested a review from ogupte June 17, 2019 07:14

ogupte approved these changes Jun 17, 2019

View reviewed changes

dgieselaar merged commit 4526e2a into elastic:master Jun 17, 2019

dgieselaar deleted the aggregation-types branch June 17, 2019 19:56

dgieselaar mentioned this pull request Jun 18, 2019

[7.x] [APM]: Inferred types for aggregations (#37360) #39148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM]: Inferred types for aggregations #37360

[APM]: Inferred types for aggregations #37360

dgieselaar commented May 29, 2019 •

edited

Loading

dgieselaar May 29, 2019

dgieselaar May 29, 2019

dgieselaar May 29, 2019

dgieselaar May 29, 2019

ogupte Jun 13, 2019

dgieselaar May 29, 2019

ogupte Jun 13, 2019

dgieselaar May 29, 2019

elasticmachine commented May 29, 2019

elasticmachine commented May 31, 2019

elasticmachine commented Jun 11, 2019

elasticmachine commented Jun 12, 2019

elasticmachine commented Jun 12, 2019

elasticmachine commented Jun 13, 2019

ogupte left a comment •

edited

Loading

ogupte Jun 13, 2019

ogupte Jun 13, 2019

dgieselaar Jun 16, 2019 •

edited

Loading

ogupte Jun 13, 2019

ogupte Jun 13, 2019

dgieselaar Jun 15, 2019 •

edited

Loading

dgieselaar Jun 17, 2019

ogupte Jun 13, 2019

dgieselaar Jun 16, 2019

ogupte Jun 17, 2019

elasticmachine commented Jun 15, 2019

elasticmachine commented Jun 16, 2019

dgieselaar commented Jun 16, 2019

elasticmachine commented Jun 16, 2019

elasticmachine commented Jun 17, 2019

ogupte left a comment

[APM]: Inferred types for aggregations #37360

[APM]: Inferred types for aggregations #37360

Conversation

dgieselaar commented May 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 29, 2019

💔 Build Failed

elasticmachine commented May 31, 2019

💔 Build Failed

elasticmachine commented Jun 11, 2019

💔 Build Failed

elasticmachine commented Jun 12, 2019

💔 Build Failed

elasticmachine commented Jun 12, 2019

💔 Build Failed

elasticmachine commented Jun 13, 2019

💚 Build Succeeded

ogupte left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar Jun 15, 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

elasticmachine commented Jun 15, 2019

💚 Build Succeeded

elasticmachine commented Jun 16, 2019

💔 Build Failed

dgieselaar commented Jun 16, 2019

elasticmachine commented Jun 16, 2019

💚 Build Succeeded

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

ogupte left a comment

Choose a reason for hiding this comment

dgieselaar commented May 29, 2019 •

edited

Loading

ogupte left a comment •

edited

Loading

dgieselaar Jun 16, 2019 •

edited

Loading

dgieselaar Jun 15, 2019 •

edited

Loading