[APM] Improve router types #83620

dgieselaar · 2020-11-18T10:13:40Z

Main changes:

Use a top-level params runtime type, instead of a plain object with a runtime type for each of path, query and body. This makes our types a little easier to maintain (as we can just use much of io-ts its tooling) and more composable.
Use a runtime type to fail on excess keys. Previously we were doing a very simple check on top-level properties, but that doesn't work anymore with the top-level runtime type. Moving it to a runtime type allows us to validate deeply nested keys as well, and test it separately.
Combine pathname and method into endpoint. Previously, if we registered two HTTP methods for the same pathname, TypeScript would either fail to resolve the route params or merge them together regardless of which one was being used. For an example, see /api/apm/settings/custom_links, where (according to TypeScript) you can leave out both body and query, or specify both, even though one and only one is required on both the GET and the POST route.

elasticmachine · 2020-11-18T10:13:42Z

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

elasticmachine · 2020-11-18T10:13:47Z

Pinging @elastic/uptime (Team:uptime)

sorenlouv · 2020-11-18T10:53:34Z

x-pack/plugins/apm/common/runtime_types/merge/index.ts

+// this is similar to t.intersection, but does a deep merge
+// instead of a shallow merge
+
+export function merge<A extends t.Mixed, B extends t.Mixed>(


Should it be called deepMerge then?
Btw. I'm surprised this doesn't already exist in the shared typescript utils.

it's merge because it uses lodash.merge. I figured deepMerge would be overkill.

sorenlouv · 2020-11-18T10:56:18Z

x-pack/plugins/apm/public/application/action_menu/anomaly_detection_setup_link.tsx

@@ -24,8 +24,7 @@ import { APIReturnType } from '../../services/rest/createCallApmApi';
 import { units } from '../../style/variables';

 export type AnomalyDetectionApiResponse = APIReturnType<
-  '/api/apm/settings/anomaly-detection',
-  'GET'
+  'GET /api/apm/settings/anomaly-detection'


Reads much nicer 👍

sorenlouv · 2020-11-18T10:57:34Z

x-pack/plugins/apm/public/application/action_menu/anomaly_detection_setup_link.tsx

@@ -60,7 +59,7 @@ export function AnomalyDetectionSetupLink() {
 export function MissingJobsAlert({ environment }: { environment?: string }) {
  const { data = DEFAULT_DATA, status } = useFetcher(
    (callApmApi) =>
-      callApmApi({ pathname: `/api/apm/settings/anomaly-detection` }),
+      callApmApi({ endpoint: `GET /api/apm/settings/anomaly-detection` }),


feels controversial. I like it!

sorenlouv · 2020-11-18T11:02:58Z

x-pack/plugins/apm/public/services/rest/createCallApmApi.ts


    const path = (params.path || {}) as Record<string, any>;
+    const [method, pathname] = endpoint.split(' ');


This will cause problems if the endpoint contains spaces. Something that's theoretically possible but widely frowned upon (for good reasons).
I don't think we need to handle this - just wanted to point it out so I can link to this conversation when this causes a production incident 5 years from now :D

I have enough faith in humanity to assume that no one will ever try to register an endpoint with spaces 😅

sorenlouv · 2020-11-18T11:07:19Z

x-pack/plugins/apm/server/routes/index_pattern.ts

-      ]),
-    }),
-  },
+export const dynamicIndexPatternRoute = createRoute({


Shouldn't we be passing processorEvent along to getDynamicIndexPattern? If not, will you remove processorEvent from getDynamicIndexPattern and getPatternIndices?

Probably also remove it from here

kibana/x-pack/plugins/apm/server/lib/helpers/setup_request.ts

Line 63 in 00c0535

processorEvent?: ProcessorEvent;

I've put it back, we were not using it but looks like it was a bug I introduced in 429805d. Previously, we were using query.processorEvent in setup_request, but we moved it out of setup_request into its own function, which required the query param to be passed explicitly, but looked like that never happened.

sorenlouv

Nice improvement, thanks!

shahzad31 · 2020-11-18T12:58:08Z

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

@@ -53,7 +53,7 @@ export function ClientMetrics() {
    (callApmApi) => {
      if (uxQuery) {
        return callApmApi({
-          pathname: '/api/apm/rum/client-metrics',
+          endpoint: 'GET /api/apm/rum/client-metrics',


i am yet to take a look at that apm client contract you have created, will these types continue to work once we move ux code out of apm?

You would have to create your own pseudo-router (create_ux_api) and client (callUxApi), but that was already the case, nothing in this PR changes what you need to do I think.

shahzad31

LGTM !!

…er-types

kibanamachine · 2020-11-18T17:46:48Z

💚 Build Succeeded

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

Metrics [docs]

Async chunks

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

id	before	after	diff
`apm`	3.1MB	3.1MB	+1.7KB

Distributable file count

id	before	after	diff
`default`	42858	42860	+2

History

💔 Build #88515 failed 3ca5000

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

* [APM] Improve router types * Pass processorEvent param to useDynamicIndexPattern

… into add-logs-to-node-details * 'add-logs-to-node-details' of github.com:phillipb/kibana: (87 commits) [Maps] Add 'crossed' & 'exited' events to tracking alert (elastic#82463) Updating code-owners to use new core/app-services team names (elastic#83731) Add Managed label to data streams and a view switch for the table (elastic#83049) [Maps] Add query bar inputs to geo threshold alerts tracked points & boundaries (elastic#80871) fix(NA): search examples kibana version declaration (elastic#83182) Fixed console error, which appears when saving changes in Edit Alert flyout (elastic#83610) [Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578) Not resetting server log level if level is defined (elastic#83651) disable incremenetal build for legacy tsconfig.json (elastic#82986) [Workplace Search] Migrate SourceLogic from ent-search (elastic#83593) [Workplace Search] Port Box changes from ent-search (elastic#83675) [APM] Improve router types (elastic#83620) Bump flat to v4.1.1 (elastic#83647) Bump y18n@5 to v5.0.5 (elastic#83644) Bump jsonpointer to v4.1.0 (elastic#83641) Bump is-my-json-valid to v2.20.5 (elastic#83642) [Telemetry] Move Monitoring collection strategy to a collector (elastic#82638) Update typescript eslint to v4.8 (elastic#83520) [ML] Persist URL state for Anomaly detection jobs using metric function (elastic#83507) [ML] Performance improvements to annotations editing in Single Metric Viewer & buttons placement (elastic#83216) ...

* [APM] Improve router types * Pass processorEvent param to useDynamicIndexPattern

[APM] Improve router types

3ca5000

dgieselaar added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 18, 2020

dgieselaar requested review from a team as code owners November 18, 2020 10:13

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Nov 18, 2020

sorenlouv reviewed Nov 18, 2020

View reviewed changes

sorenlouv approved these changes Nov 18, 2020

View reviewed changes

shahzad31 reviewed Nov 18, 2020

View reviewed changes

shahzad31 approved these changes Nov 18, 2020

View reviewed changes

dgieselaar added 2 commits November 18, 2020 16:39

Merge branch 'master' of github.com:elastic/kibana into simplify-rout…

b0aa5f9

…er-types

Pass processorEvent param to useDynamicIndexPattern

d992070

dgieselaar merged commit 2a365ff into elastic:master Nov 18, 2020

dgieselaar deleted the simplify-router-types branch November 18, 2020 18:50

dgieselaar mentioned this pull request Nov 18, 2020

[7.x] [APM] Improve router types (#83620) #83699

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Nov 18, 2020

[APM] Improve router types (elastic#83620)

6518ac0

* [APM] Improve router types * Pass processorEvent param to useDynamicIndexPattern

dgieselaar added a commit that referenced this pull request Nov 18, 2020

[APM] Improve router types (#83620) (#83699)

5da99bb

* [APM] Improve router types * Pass processorEvent param to useDynamicIndexPattern

chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 19, 2020

[APM] Improve router types (elastic#83620)

825837b

* [APM] Improve router types * Pass processorEvent param to useDynamicIndexPattern

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Improve router types #83620

[APM] Improve router types #83620

dgieselaar commented Nov 18, 2020

elasticmachine commented Nov 18, 2020

elasticmachine commented Nov 18, 2020

sorenlouv Nov 18, 2020

dgieselaar Nov 18, 2020

sorenlouv Nov 18, 2020

sorenlouv Nov 18, 2020

sorenlouv Nov 18, 2020 •

edited

Loading

dgieselaar Nov 18, 2020

sorenlouv Nov 18, 2020 •

edited

Loading

sorenlouv Nov 18, 2020

dgieselaar Nov 18, 2020

sorenlouv left a comment

shahzad31 Nov 18, 2020

dgieselaar Nov 18, 2020

shahzad31 left a comment

kibanamachine commented Nov 18, 2020


		const path = (params.path \|\| {}) as Record<string, any>;
		const [method, pathname] = endpoint.split(' ');

[APM] Improve router types #83620

[APM] Improve router types #83620

Conversation

dgieselaar commented Nov 18, 2020

elasticmachine commented Nov 18, 2020

elasticmachine commented Nov 18, 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

sorenlouv Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 18, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

History

sorenlouv Nov 18, 2020 •

edited

Loading

sorenlouv Nov 18, 2020 •

edited

Loading