[Uptime] Remove legacy es client #51403

justinkambic · 2019-11-21T23:43:55Z

Summary

~~Working toward removing legacy elasticsearch client from Uptime backend.~~

~~Code is WIP.~~

~~Do not review until #51125 is merged.~~

The biggest change in this patch is that we are getting our elasticsearch client from the request object provided by the Kibana route handler. Before we were storing references to an elasticsearch client that was provided at bootstrap time.

Changes

GraphQL route creation

We were previously passing the request object to our GraphQL resolvers by including it in an options object that was included in the resolver parameters. We're doing essentially the same thing, but now are including the callWithCurrentUser function provided by the request context.

The implementation of this is largely the same in GQL resolvers, except that we're simply passing this client to the adapters instead of the request object itself. Example:

Before:

async getStatesIndexStatus(resolver, {}, { req }): Promise<StatesIndexStatus> {
  return await libs.monitorStates.statesIndexExists(req);
},

After:

async getStatesIndexStatus(_resolver, {}, { callAsCurrentUser }): Promise<StatesIndexStatus> {
  return await libs.monitorStates.statesIndexExists(callAsCurrentUser, undefined);
},

Adapter interfaces

Adapter functions now subscribe to a shared interface. Their first parameter is the elasticsearch client used by the adapter to query ES. The second parameter is either an object containing data needed for the adapter query, or undefined. I might revise this before opening the PR for review to take only a shared object.

This interface can be found here.

Before:

const result = await libs.monitorStates.getSnapshotCount(
  request,
  dateRangeStart,
  dateRangeEnd,
  filters,
  statusFilter
);

After:

const result = await libs.monitorStates.getSnapshotCount(callAsCurrentUser, {
  dateRangeStart,
  dateRangeEnd,
  filters,
  statusFilter,
});

The goal is to eventually extract all these methods out of their current class configuration into pure functions exported from their own modules, which will make them simpler to test and create shorter adapter files.

Reviewing the PR

Primary changes here are architectural - if all tests are passing and you're able to open/run the Uptime app like it would normally work, all should be well. The larger portion of the review should be to ensure that I've preserved existing code quality as much as possible, and that new patterns introduced make sense.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-11-21T23:43:57Z

Pinging @elastic/uptime (Team:uptime)

elasticmachine · 2019-11-22T00:34:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 163596727c5e58f46b7d62fb9e57d206afdb6db6

elasticmachine · 2019-11-22T20:00:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 8db19a0a1980d53f8a3095ae25b9f9d91a518787

elasticmachine · 2019-11-22T22:57:33Z

💚 Build Succeeded

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

elasticmachine · 2019-11-25T20:50:37Z

💔 Build Failed

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

elasticmachine · 2019-11-26T15:59:25Z

💚 Build Succeeded

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

elasticmachine · 2019-12-03T17:02:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 84fdd3d2c08bb4d1fd3ec77016b60f4a542dea15

elasticmachine · 2019-12-04T00:09:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5267e09b42032bada43153a69dbfb2c823967f50

… resp body.

…itor details endpoint.

andrewvc

Generally looks great, though I did have some questions about some aspects of the code.

Code runs fine in a smoke test locally.

justinkambic · 2019-12-10T16:59:18Z

@elasticmachine merge upstream

shahzad31 · 2019-12-12T11:39:50Z

x-pack/legacy/plugins/uptime/server/lib/adapters/monitor_states/adapter_types.ts

+export interface GetSnapshotCountParams {
+  dateRangeStart: string;
+  dateRangeEnd: string;
+  filters?: string | null;
+  statusFilter?: string | null;
+}
+


it's generally makes sense to move all the interfaces to the top of the file, can you move this close to MonitorStatesParams?
And also may be use MonitorStatesParams for snapshot as well, since pagination is optional. Once you do that i guess it will also makes sense to have a bit more generic name Just QueryParams, WDYT?

We already have that in the first Maps PR, when we merge one we can overwrite this for the other and yes we can just use a generic interface for components that only require these four basic params I agree!

shahzad31 · 2019-12-12T11:44:19Z

...cy/plugins/uptime/server/lib/adapters/monitor_states/elasticsearch_monitor_states_adapter.ts

+    dateRangeEnd,
+    pagination,
+    filters,
+    statusFilter,


Probably not related to this PR, but i have always been confused by filters and statusFilter, i mean they always gives my brain a challenge to process the information. "Ok we have filters and then we have statusFilter :) let's think about it , what they means."
i guess we should probably rename statusFilter to monitorStatus or maybe just merge it with filters.or maybe it's fine, it's just me :D

We should chat about this, I'm realizing maybe @andrewvc and myself have never explained why we have this weirdness. I'd be fine to rename it to something else in a separate PR if we can think of a less confusing way to refer to it.

shahzad31 · 2019-12-12T11:52:44Z

...cy/plugins/uptime/server/lib/adapters/monitor_states/elasticsearch_monitor_states_adapter.ts

      filterClause: filters && filters !== '' ? JSON.parse(filters) : null,
      size: CONTEXT_DEFAULTS.MAX_MONITORS_FOR_SNAPSHOT_COUNT,
-      statusFilter,
+      statusFilter: statusFilter || undefined,


i wonder why are you explicitly assigning it as undefined, if it's already undefined, i mean that's how short-hand objection notation works.

Reason for this: QueryContext requires statusFilter to be string | undefined, but the typing for statusFilter from API requests historically can be string | null | undefined. I didn't want to break any code that might be explicitly testing the value for undefined so I overwrite any falsy value with undefined. We can clean this up by modifying the type provided by the request handler, which I will do.

Fixed this in c0d5187.

shahzad31 · 2019-12-12T12:06:55Z

x-pack/legacy/plugins/uptime/server/lib/adapters/pings/adapter_types.ts

+      value: number;
+    };
+  };
 }


Another unrelated to PR , suggestion is to rename adapter_types file to just types.

👍 this naming convention predates Uptime and was a result of mirroring other new plugins. I think we've done away with many of the conventions, so I am fine with renaming these files to make them less verbose.

I created elastic/uptime#126

shahzad31 · 2019-12-12T12:10:41Z

x-pack/legacy/plugins/uptime/server/lib/adapters/pings/elasticsearch_pings_adapter.ts

-  }
-}
+  },
+};


Again file naming, unrelated to this PR, elasticsearch_pings_adapter.ts should be just elasticsearch_pings, since it's inside adapters folder, so it doesn't make sense to prefix/postfix files. It's a twitter rules as well, when you have already used "lol", "haha" can't be used in the same tweet :P

Lol, I see what you're saying haha.

Addressed in #51403 (comment).

shahzad31

i have left some suggestions, those are totally optional.
other PR is good to go, smoke tested and didn't find any issue.

…bic/kibana into uptime_remove-legacy-es-client

elasticmachine · 2019-12-17T16:13:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 75e9d84

History

💔 Build #16084 failed c745fb8
💚 Build #15922 succeeded 2300531
💚 Build #15207 succeeded 07f9cc4
💚 Build #14574 succeeded ed9d586
💚 Build #14385 succeeded ac4e2e9

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

andrewvc

LGTM

* Move a REST endpoint and the GQL endpoint to NP routing. * Delete obsolete REST endpoint. * Update remaining REST routes to work with NP router. * Remove obsolete code, update some unit tests. * Simplify route creation. * Remove tests of API decommissioned API endpoint. * Rename domain check. * Make return shape of index pattern endpoint correspond to required NP resp body. * Move validate to appropriate level of route definition object for monitor details endpoint. * Update snapshot count route. * Fix broken lint rule. * Move a REST endpoint and the GQL endpoint to NP routing. * Update remaining REST routes to work with NP router. * Update remaining REST routes to work with NP router. * Refactor query functions to accept new es client from request contexts. * WIP updating framework adapter. * Refactor remaining routes/resolvers to remove usage of legacy Elasticsearch client. * Fix broken unit tests. * Fix incorrect user usage for a REST endpoint. * Fix some broken imports and types. * Port monitor details REST endpoint to NP. * Remove some merge errors. * Update adapters to take a single options parameter. * Update broken test files. * Resolve typescript warnings. * Update resolver types. * Change GraphQL interface name for es client. * Delete unused code and fix incorrect type. * Rename type for REST endpoint creators. * Nest message values in body object for invalid response messages. * Reorganize a file and clean up some types. * Add wrapper function to reduce boilerplate route code.

justinkambic · 2019-12-19T21:49:00Z

Backported to:
7.x/7.6.0 c620d11
#53615

justinkambic added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 21, 2019

justinkambic self-assigned this Nov 21, 2019

justinkambic force-pushed the uptime_remove-legacy-es-client branch from b62319c to 84fdd3d Compare December 3, 2019 15:46

justinkambic force-pushed the uptime_remove-legacy-es-client branch from 84fdd3d to 5267e09 Compare December 3, 2019 22:51

justinkambic added 15 commits December 4, 2019 15:25

Move a REST endpoint and the GQL endpoint to NP routing.

0314775

Delete obsolete REST endpoint.

1dd6ea8

Update remaining REST routes to work with NP router.

a05594d

Remove obsolete code, update some unit tests.

74bb6f0

Simplify route creation.

b2730e5

Remove tests of API decommissioned API endpoint.

c36e42a

Rename domain check.

92aee58

Make return shape of index pattern endpoint correspond to required NP…

49d8cf2

… resp body.

Move validate to appropriate level of route definition object for mon…

bb35540

…itor details endpoint.

Update snapshot count route.

789d232

Fix broken lint rule.

a4047af

Move a REST endpoint and the GQL endpoint to NP routing.

7221e8a

Update remaining REST routes to work with NP router.

64fe387

Refactor query functions to accept new es client from request contexts.

43a1c83

WIP updating framework adapter.

6eb1bf3

Delete unused code and fix incorrect type.

d7601be

andrewvc reviewed Dec 9, 2019

View reviewed changes

justinkambic added 2 commits December 9, 2019 16:55

Rename type for REST endpoint creators.

17a6adc

Nest message values in body object for invalid response messages.

ac4e2e9

justinkambic requested a review from andrewvc December 9, 2019 23:18

Merge branch 'master' into uptime_remove-legacy-es-client

ed9d586

shahzad31 reviewed Dec 12, 2019

View reviewed changes

shahzad31 approved these changes Dec 12, 2019

View reviewed changes

Merge branch 'master' into uptime_remove-legacy-es-client

c440c64

justinkambic mentioned this pull request Dec 12, 2019

Give server lib files less verbose names elastic/uptime#126

Closed

justinkambic added 5 commits December 12, 2019 13:04

Reorganize a file and clean up some types.

c0d5187

Merge branch 'uptime_remove-legacy-es-client' of github.com:justinkam…

07f9cc4

…bic/kibana into uptime_remove-legacy-es-client

Merge master, modify new route to conform to updated ES client calling.

2300531

Add wrapper function to reduce boilerplate route code.

c745fb8

Merge branch 'master' into uptime_remove-legacy-es-client

75e9d84

andrewvc approved these changes Dec 17, 2019

View reviewed changes

justinkambic merged commit b7ff35d into elastic:master Dec 17, 2019

justinkambic mentioned this pull request Dec 19, 2019

[7.x] [Uptime] Remove legacy es client (#51403) #53615

Merged

justinkambic deleted the uptime_remove-legacy-es-client branch December 19, 2019 21:49

justinkambic mentioned this pull request Dec 19, 2019

[Uptime] Reimplement saved objects client creation #49979

Closed

justinkambic mentioned this pull request Jan 28, 2020

[Uptime] Saved Objects interaction is performed with an unnecessary custom client #49977

Closed

[Uptime] Remove legacy es client #51403

[Uptime] Remove legacy es client #51403

Uh oh!

Conversation

justinkambic commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

GraphQL route creation

Adapter interfaces

Reviewing the PR

Checklist

For maintainers

Uh oh!

elasticmachine commented Nov 21, 2019

Uh oh!

elasticmachine commented Nov 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 22, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Nov 25, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 26, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Dec 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Dec 4, 2019

💔 Build Failed

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

justinkambic commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahzad31 left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 17, 2019

💚 Build Succeeded

History

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

justinkambic commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

justinkambic commented Nov 21, 2019 •

edited

Loading