[APM] Improve type safety across application boundaries by sorenlouv · Pull Request #42252 · elastic/kibana

sorenlouv · 2019-07-30T13:22:25Z

This PR does two things

Move the exported type definition to the route layer
Enables type checking of API query params between frontend and backend

Problem:
We want to have type safety between the client and the server. We currently infer the response of a rest call by referring to the exported type on the server side:

callApi<SomeTypeFromBackend>(...)

There are are two problems with this:

The exported type SomeTypeFromBackend is exported from deep inside the server application, and thus does take possible transformations at the routing layer into consideration
The exported type only contains information about the return type - nothing else about the API (eg query params, method etc)

Solution
Instead of referring to a specific return type the client should simply refer to the route method it is calling:

callApi<typeof transaction_distribution_route>(...)

transaction_distribution_route is the name of the route. This is at the very edge of the server side application so we can be much more confident that the return type of this method corresponds to what is actually transmitted over the wire.
Additionally, since we are now passing the type of the backend route we can infer more than just the return type. Currently I've added inference of query params but in the future we might also be able to infer method and perhaps do some magic with path.

I've only converted the transaction_groups APIs. I'd like to get feedback before converting the remaining APIs. I don't expect that to take more than half a day.

elasticmachine · 2019-07-30T13:58:30Z

Pinging @elastic/apm-ui

elasticmachine · 2019-07-30T14:21:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-07-30T14:59:33Z

x-pack/legacy/plugins/apm/public/services/rest/apm/transaction_groups.ts

Instead of referring to a type I refer to a routing function.

dgieselaar · 2019-07-30T15:04:30Z

Pretty stoked about this! I've been thinking about this as well, for instance the gap between Joi validation and query parameters feels brittle. I think we could have some kind of DSL that both TypeScript and Joi can handle, that would remove the need for at least some duplication. Another thing that I considered was to have static strings always for an API call, so /api/apm/services/:serviceName/name_of_endpoint would become /api/apm/services/name_of_endpoint with a query parameter serviceName. The callApi method would then no longer need a generic argument, it could just infer it from the url and add validation/autocompletion for parameters passed to it as well:

return await callApi('/api/apm/transaction_groups/distribution', { });

Maybe a little overkill though 😅

elasticmachine · 2019-07-30T16:08:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-07-31T10:06:45Z

for instance the gap between Joi validation and query parameters feels brittle. I think we could have some kind of DSL that both TypeScript and Joi can handle, that would remove the need for at least some duplication

Yes, there is definitely room for improvement. If I added all the things I wanted it would become a sinkhole :D I feel this is a pretty minimal first step, that will allow us to iterate on it. The typescript magic I added is still fairly simple. If we want to go more advanced we might want to look at typesafe-hapi so we don't duplicate that work. But that's a much bigger change if we were to convince Kibana to switch.

sorenlouv · 2019-07-31T10:09:34Z

Another thing that I considered was to have static strings always for an API call, so /api/apm/services/:serviceName/name_of_endpoint would become /api/apm/services/name_of_endpoint with a query parameter serviceName. The callApi method would then no longer need a generic argument, it could just infer it from the url and add validation/autocompletion for parameters passed to it as well.

That's an interesting thought. I definitely see the benefits of static endpoints. Currently our client side routes and server side APIs map almost 1:1 so we might want to change those as well for consistency.

elasticmachine · 2019-07-31T12:55:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-31T15:07:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-01T13:56:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-01T14:05:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-01T15:09:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-02T07:35:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

dgieselaar

LGTM, just a few nits/questions

x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/default.ts

dgieselaar · 2019-08-02T10:04:01Z

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


 export async function getJavaMetricsCharts(setup: Setup, serviceName: string) {
-  const charts = await Promise.all([
+  const charts: GenericMetricsChart[] = await Promise.all([


same here, curious why we need it

It was previously used here:

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

Lines 11 to 13 in 33c237e

export interface MetricsChartsByAgentAPIResponse {

charts: GenericMetricsChart[];

}

Removing it caused typescript errors - often this happens when a const like type: 'my-type' is converted to type: string. Re-adding GenericMetricsChart solved it.

If you find a way to remove it and still keep typescript happy I'll gladly do that.

Yeah, removing this causes this error:

legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts:14:3 - error TS2322: Type '[]' is not assignable to type '[{ title: string; key: string; yUnit: YUnit; totalHits: number; series: { title: string; key: string; type: ChartType; color: string; overallValue: number | null; data: { x: number; y: number | null; }[]; }[]; }, { ...; }, { ...; }, { ...; }, { ...; }] | [...]'. Type '[]' is missing the following properties from type '[{ title: string; key: string; yUnit: YUnit; totalHits: number; series: { title: string; key: string; type: ChartType; color: string; overallValue: number | null; data: { x: number; y: number | null; }[]; }[]; }, { ...; }]': 0, 1 14 charts: [] ~~~~~~ legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts:24:12 24 return { charts }; ~~~~~~ The expected type comes from property 'charts' which is declared here on type '{ charts: [{ title: string; key: string; yUnit: YUnit; totalHits: number; series: { title: string; key: string; type: ChartType; color: string; overallValue: number | null; data: { x: number; y: number | null; }[]; }[]; }, { ...; }, { ...; }, { ...; }, { ...; }]; } | { ...; }' legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts:13:1 - error TS6133: 'GenericMetricsChart' is declared but its value is never read. 13 import { GenericMetricsChart } from '../../transform_metrics_chart'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/x-pack/legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts b/x-pack/legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts index 3480ede7ac..b1b0315b00 100644 --- a/x-pack/legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts +++ b/x-pack/legacy/plugins/apm/public/hooks/useServiceMetricCharts.ts @@ -10,8 +10,10 @@ import { useUiFilters } from '../context/UrlParamsContext'; import { useFetcher } from './useFetcher'; import { PromiseReturnType } from '../../typings/common'; -const INITIAL_DATA: PromiseReturnType<typeof loadMetricsChartData> = { - charts: [] +type MetricsChartDataResponse = PromiseReturnType<typeof loadMetricsChartData>; + +const INITIAL_DATA = { + charts: [] as Array<MetricsChartDataResponse['charts'][0]> }; export function useServiceMetricCharts( diff --git a/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/default.ts b/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/default.ts index 453ba614f2..f55d3320c7 100644 --- a/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/default.ts +++ b/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/default.ts @@ -7,13 +7,12 @@ import { Setup } from '../../helpers/setup_request'; import { getCPUChartData } from './shared/cpu'; import { getMemoryChartData } from './shared/memory'; -import { GenericMetricsChart } from '../transform_metrics_chart'; export async function getDefaultMetricsCharts( setup: Setup, serviceName: string ) { - const charts: GenericMetricsChart[] = await Promise.all([ + const charts = await Promise.all([ getCPUChartData(setup, serviceName), getMemoryChartData(setup, serviceName) ]); diff --git a/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts b/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts index 7ea1d0bf69..9fc9f3029d 100644 --- a/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts +++ b/x-pack/legacy/plugins/apm/server/lib/metrics/by_agent/java/index.ts @@ -10,10 +10,9 @@ import { getNonHeapMemoryChart } from './non_heap_memory'; import { getThreadCountChart } from './thread_count'; import { getCPUChartData } from '../shared/cpu'; import { getMemoryChartData } from '../shared/memory'; -import { GenericMetricsChart } from '../../transform_metrics_chart'; export async function getJavaMetricsCharts(setup: Setup, serviceName: string) { - const charts: GenericMetricsChart[] = await Promise.all([ + const charts = await Promise.all([ getCPUChartData(setup, serviceName), getMemoryChartData(setup, serviceName), getHeapMemoryChart(setup, serviceName),

x-pack/legacy/plugins/apm/server/lib/transactions/distribution/get_buckets/fetcher.ts

dgieselaar · 2019-08-02T10:17:37Z

x-pack/legacy/plugins/apm/server/routes/__test__/routeFailures.test.ts

-          isBoom: true
+          message: 'request failed'
        });
-        expect(consoleErrorSpy).toHaveBeenCalledTimes(1);


why are we removing this assertion?

presumably because of 265cc3b?

Yes, exactly. Removing defaultErrorHandler was perhaps a bit out of the scope for this PR so I can re-add it if you think. Although I don't think it provided us with much (and now the status code is correctly 500 instead of 400).

elasticmachine · 2019-08-02T13:37:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2019-08-06T02:29:49Z

Yes, there is definitely room for improvement. If I added all the things I wanted it would become a sinkhole :D I feel this is a pretty minimal first step, that will allow us to iterate on it. The typescript magic I added is still fairly simple. If we want to go more advanced we might want to look at typesafe-hapi so we don't duplicate that work. But that's a much bigger change if we were to convince Kibana to switch.

Hey just for the record, @weltenwort and some of us on infra/logs are evaluating io-ts https://github.com/gcanti/io-ts to try to tackle this problem. See #41836 and #42356 for our first attempts to implement and see if it's worth the DSL. I like what you all have here too, it'd be great if we found a solution that we all used at least across o11y and maybe beyond!

…nd-to-frontend-contract

elasticmachine · 2019-08-06T16:15:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

dgieselaar · 2019-08-24T11:23:48Z

Closing, work was done in #42961.

sorenlouv requested a review from a team July 30, 2019 13:22

sorenlouv added release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. labels Jul 30, 2019

sorenlouv commented Jul 30, 2019

View reviewed changes

sorenlouv requested review from dgieselaar and ogupte July 30, 2019 15:02

sorenlouv added 2 commits July 31, 2019 13:53

[APM] Type checking of query params

2e25b6a

Conditionally add should params in transaction distribution ES query

7f7cfbc

sorenlouv force-pushed the improve-backend-to-frontend-contract branch from 72871b0 to 7f7cfbc Compare July 31, 2019 11:53

sorenlouv added the [zube]: In Progress label Jul 31, 2019

sorenlouv self-assigned this Jul 31, 2019

sorenlouv added 3 commits July 31, 2019 15:19

Convert errors

2ef4a4e

Converted index pattern api

ebe4ad9

Introduce callApmApi

83bddf9

sorenlouv added 4 commits August 1, 2019 10:53

Fix route spec

66c7c15

Remove defaultErrorHandler

265cc3b

Migrate metrics

89b1f9e

Migrate remaining routes

8842233

sorenlouv force-pushed the improve-backend-to-frontend-contract branch from e883295 to 8842233 Compare August 1, 2019 13:02

fix api test

4a5e6c7

sorenlouv changed the title ~~[APM] Improve type safety across application boundaries (POC)~~ [APM] Improve type safety across application boundaries Aug 1, 2019

fix api test message

f251354

dgieselaar approved these changes Aug 2, 2019

View reviewed changes

Replace mutations with ternaries

aad2fc0

sorenlouv mentioned this pull request Aug 6, 2019

[APM] Local UI filters #41588

Merged

4 tasks

Merge branch 'master' of github.com:elastic/kibana into improve-backe…

c9a93b7

…nd-to-frontend-contract

sorenlouv assigned dgieselaar and unassigned sorenlouv and dgieselaar Aug 14, 2019

dgieselaar closed this Aug 24, 2019

zube bot added [zube]: Done and removed [zube]: In Progress labels Aug 24, 2019

sorenlouv deleted the improve-backend-to-frontend-contract branch September 25, 2019 12:25

sorenlouv removed the [zube]: Done label Dec 4, 2019

	export interface MetricsChartsByAgentAPIResponse {
	charts: GenericMetricsChart[];
	}

Conversation

sorenlouv commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 30, 2019

Uh oh!

elasticmachine commented Jul 30, 2019

💔 Build Failed

Uh oh!

sorenlouv Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgieselaar commented Jul 30, 2019

Uh oh!

elasticmachine commented Jul 30, 2019

💔 Build Failed

Uh oh!

sorenlouv commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sorenlouv commented Jul 31, 2019

Uh oh!

elasticmachine commented Jul 31, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 31, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Aug 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Aug 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Aug 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

Uh oh!

dgieselaar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dgieselaar Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

sorenlouv Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

sorenlouv Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

dgieselaar Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dgieselaar Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

dgieselaar Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

Uh oh!

jasonrhodes commented Aug 6, 2019

Uh oh!

elasticmachine commented Aug 6, 2019

💚 Build Succeeded

Uh oh!

sorenlouv commented Jul 30, 2019 •

edited

Loading

sorenlouv Jul 30, 2019 •

edited

Loading

sorenlouv commented Jul 31, 2019 •

edited

Loading

sorenlouv Aug 2, 2019 •

edited

Loading

dgieselaar Aug 2, 2019 •

edited

Loading