[Watcher] Migrate to new ES client #97260

jloleysens · 2021-04-15T14:24:18Z

Summary

How to test

Start Kibana and ES and activate your trial license in the License Management
Follow the UI for creating both an advanced watch and a threshold watch
- For the threshold set a very low document count threshold for the .kibana index so that the alert will fire
- Add a logging action for the threshold watch
- Ensure that the watch will fire frequently (every 5 seconds or so)
Go the the home screen of the watcher management section and click on the threshold watch
Once the watch has entered "Firing" state attempt to acknowledge the alert

IScopedClusterClient and from "isEsError" to "handleEsError"

elasticmachine · 2021-04-15T16:07:48Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

…te-legacy-es-client * 'master' of github.com:elastic/kibana: (102 commits) [Exploratory view] integerate page views to exploratory view (elastic#97258) Fix typo in license_api_guard README name and import http server mocks from public interface (elastic#97334) Avoid mutating KQL query when validating it (elastic#97081) Add description as title on tag badge (elastic#97109) Remove legacy ES client usages in `home` and `xpack_legacy` (elastic#97359) [Fleet] Finer-grained error information from install/upgrade API (elastic#95649) Rule registry bundle size (elastic#97251) [Partial Results] Move other bucket into Search Source (elastic#96384) [Dashboard] Makes lens default editor for creating new panels (elastic#96181) skip flaky suite (elastic#97387) [Asset Management] Agent picker follow up (elastic#97357) skip flaky suite (elastic#97382) [Security Solutions] Fixes flake with cypress tests (elastic#97329) skip flaky suite (elastic#97355) Skip test to try and stabilize master minimize number of so fild asserted in tests. it creates flakines when implementation details change (elastic#97374) [Search Sessions] Client side search cache (elastic#92439) [SavedObjects] Add aggregations support (elastic#96292) [Reporting] Remove legacy elasticsearch client usage from the reporting plugin (elastic#97184) [kbnClient] fix basePath handling and export reponse type (elastic#97277) ... # Conflicts: # x-pack/plugins/watcher/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts # x-pack/plugins/watcher/server/plugin.ts # x-pack/plugins/watcher/server/routes/api/indices/register_get_route.ts # x-pack/plugins/watcher/server/routes/api/license/register_refresh_route.ts # x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts # x-pack/plugins/watcher/server/routes/api/register_load_history_route.ts # x-pack/plugins/watcher/server/routes/api/settings/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/action/register_acknowledge_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_activate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_deactivate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_execute_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_history_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_save_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_visualize_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_list_route.ts # x-pack/plugins/watcher/server/shared_imports.ts # x-pack/plugins/watcher/server/types.ts

jloleysens · 2021-04-21T10:15:28Z

@elasticmachine merge upstream

jloleysens · 2021-04-28T10:06:48Z

@elasticmachine merge upstream

cjcenizal · 2021-04-28T23:19:39Z

@elasticmachine merge upstream

cjcenizal

Tested locally, code LGTM! Had a few comments and questions, but nothing worth blocking.

I tested the following behavior:

Create watch
Edit watch
Fetch execution history
Fetch action statuses
Activate watch
Deactivate watch
Delete watch

I didn't get to test the acknowledge route, but will do so tomorrow.

cjcenizal · 2021-04-28T22:54:39Z

x-pack/plugins/watcher/server/lib/fetch_all_from_scroll/fetch_all_from_scroll.ts

-): Promise<any> {
-  const newHits = get(searchResuls, 'hits.hits', []);
-  const scrollId = get(searchResuls, '_scroll_id');
+  searchResults: ScrollResponse<unknown>,


Nice typo catch!

cjcenizal · 2021-04-28T22:55:27Z

x-pack/plugins/watcher/server/lib/fetch_all_from_scroll/fetch_all_from_scroll.test.js

@@ -33,9 +29,9 @@ describe('fetch_all_from_scroll', () => {
        });
      });

-      it('should not call callWithRequest', () => {
+      it('should not call asCurrentUser.scroll', () => {


I like how these tests became clearer as a result of this migration!

cjcenizal · 2021-04-28T23:06:29Z

x-pack/plugins/watcher/server/routes/api/indices/register_get_route.ts

-        return response.aggregations.indices.buckets.map((bucket: any) => bucket.key);
-      });
-    });
+  return indices.buckets ? indices.buckets.map((bucket) => bucket.key) : [];


I was going to suggest being even stricter here:

return Array.isArray(indices.buckets) ? indices.buckets.map((bucket) => bucket.key) : [];

I'm imagining the response containing something unexpected, either because of a deliberate API change or because of a bug, and generating a 500 because map is undefined. But then I decided that it will be easier to debug a user complaining about a 500 and the accompanying stack trace in the logs, than it would be to debug a user complaining about silently disappearing information. So I think this is good as-is. :)

cjcenizal · 2021-04-28T23:09:33Z

x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts

+    {
+      index: indexes,
+      fields: ['*'],
+      allow_no_indices: true,


Looks like the ignoreUnavailable param was omitted. Was that deliberate?

Nope this was a mistake!

cjcenizal · 2021-04-28T23:11:33Z

x-pack/plugins/watcher/server/routes/api/watch/action/register_acknowledge_route.ts

-        // Case: default
-        throw e;
+        // TODO: Figure out if this covers us sufficiently given that previous logic returned a body with "Watch with id = ${watchId} not found" previously
+        return handleEsError({ error: e, response });


Did you look into how to test this, either in this handler or in the other route handlers that contain this TODO?

Yeah, I just checked this out and it looks like the ES response we get looks like:

Which is not very user friendly. So I reintroduced the renaming logic so that the response for 404s is now:

cjcenizal · 2021-04-28T23:15:09Z

x-pack/plugins/watcher/server/routes/api/watch/register_visualize_route.ts

+      {
+        index,
+        body,
+        allow_no_indices: true,


I guess the decision to omit ignoreUnavailable was deliberate?

Same as above, will re-add

cjcenizal · 2021-04-28T23:18:56Z

x-pack/plugins/watcher/server/routes/api/watches/register_list_route.ts

+      },
+      { ignore: [404] }
+    )
+    .then(({ body: response }) => fetchAllFromScroll(response, dataClient));


Nit: why not pass body as-is instead of renaming it?

.then(({ body }) => fetchAllFromScroll(body, dataClient));

jloleysens · 2021-04-29T13:37:18Z

@elasticmachine merge upstream

alisonelizabeth · 2021-04-29T13:46:47Z

It's awesome to see yet another plugin migrated to use the new ES client 🎉 ! Nice job working through these. I was wondering if we can also delete watcher/server/lib/elasticsearch_js_plugin.ts as part of this work?

…g TODOs

jloleysens · 2021-04-29T14:15:42Z

we can also delete watcher/server/lib/elasticsearch_js_plugin.ts as part of this work?

I always leave this for last for some reason 😅 , seems like to should be the first order of business.

kibanamachine · 2021-04-29T16:14:15Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #123222 succeeded 7ecbc00
💚 Build #123128 succeeded 82b522e
💚 Build #122918 succeeded 0a8f2d9
💛 Build #121334 was flaky 6184d47
💚 Build #120642 succeeded 9a429d5

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

* initial migration away from ILegacyScopedClusterClient to IScopedClusterClient and from "isEsError" to "handleEsError" * re-instate ignore: [404] * remove use of ignore_unavailable * get the correct payload from the response * fix use of new licensePreRoutingFactory * fix jest tests * address CJs feedback and re-add ignore_unavailable, clean up remaining TODOs * remove legacy client config * undo renaming as part of destructuring assignment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* initial migration away from ILegacyScopedClusterClient to IScopedClusterClient and from "isEsError" to "handleEsError" * re-instate ignore: [404] * remove use of ignore_unavailable * get the correct payload from the response * fix use of new licensePreRoutingFactory * fix jest tests * address CJs feedback and re-add ignore_unavailable, clean up remaining TODOs * remove legacy client config * undo renaming as part of destructuring assignment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* initial migration away from ILegacyScopedClusterClient to IScopedClusterClient and from "isEsError" to "handleEsError" * re-instate ignore: [404] * remove use of ignore_unavailable * get the correct payload from the response * fix use of new licensePreRoutingFactory * fix jest tests * address CJs feedback and re-add ignore_unavailable, clean up remaining TODOs * remove legacy client config * undo renaming as part of destructuring assignment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jloleysens added 4 commits April 15, 2021 15:49

initial migration away from ILegacyScopedClusterClient to

2157b43

IScopedClusterClient and from "isEsError" to "handleEsError"

re-instate ignore: [404]

9e96b76

remove use of ignore_unavailable

9568b3f

get the correct payload from the response

cc3e31b

jloleysens added Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Apr 15, 2021

jloleysens requested a review from cjcenizal April 15, 2021 14:24

jloleysens mentioned this pull request Apr 15, 2021

Stack Management elasticsearch-js client migration #73973

Closed

17 tasks

fix use of new licensePreRoutingFactory

8ed2758

jloleysens marked this pull request as ready for review April 15, 2021 16:07

jloleysens requested a review from a team as a code owner April 15, 2021 16:07

jloleysens added 2 commits April 19, 2021 12:04

fix jest tests

9a429d5

Merge branch 'master' into watcher/migrate-legacy-es-client

6184d47

jloleysens added v7.14.0 and removed v7.13.0 labels Apr 21, 2021

alisonelizabeth mentioned this pull request Apr 23, 2021

[Meta] Switch to the new Elasticsearch client #83910

Closed

74 tasks

Merge branch 'master' into watcher/migrate-legacy-es-client

0a8f2d9

Merge branch 'master' into watcher/migrate-legacy-es-client

82b522e

cjcenizal approved these changes Apr 28, 2021

View reviewed changes

Merge branch 'master' into watcher/migrate-legacy-es-client

7ecbc00

address CJs feedback and re-add ignore_unavailable, clean up remainin…

1198c3b

…g TODOs

jloleysens added 2 commits April 29, 2021 16:16

remove legacy client config

f4220e0

undo renaming as part of destructuring assignment

8c22010

jloleysens merged commit e297fec into elastic:master Apr 30, 2021

jloleysens deleted the watcher/migrate-legacy-es-client branch April 30, 2021 08:44

jloleysens mentioned this pull request Apr 30, 2021

[7.x] [Watcher] Migrate to new ES client (#97260) #98878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Watcher] Migrate to new ES client #97260

[Watcher] Migrate to new ES client #97260

jloleysens commented Apr 15, 2021 •

edited

Loading

elasticmachine commented Apr 15, 2021

jloleysens commented Apr 21, 2021

jloleysens commented Apr 28, 2021

cjcenizal commented Apr 28, 2021

cjcenizal left a comment

cjcenizal Apr 28, 2021

cjcenizal Apr 28, 2021

cjcenizal Apr 28, 2021

cjcenizal Apr 28, 2021

jloleysens Apr 29, 2021

cjcenizal Apr 28, 2021

jloleysens Apr 29, 2021

cjcenizal Apr 29, 2021

cjcenizal Apr 28, 2021

jloleysens Apr 29, 2021

cjcenizal Apr 28, 2021

jloleysens commented Apr 29, 2021

alisonelizabeth commented Apr 29, 2021

jloleysens commented Apr 29, 2021

kibanamachine commented Apr 29, 2021

[Watcher] Migrate to new ES client #97260

[Watcher] Migrate to new ES client #97260

Conversation

jloleysens commented Apr 15, 2021 • edited Loading

Summary

How to test

elasticmachine commented Apr 15, 2021

jloleysens commented Apr 21, 2021

jloleysens commented Apr 28, 2021

cjcenizal commented Apr 28, 2021

cjcenizal 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

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

jloleysens commented Apr 29, 2021

alisonelizabeth commented Apr 29, 2021

jloleysens commented Apr 29, 2021

kibanamachine commented Apr 29, 2021

💚 Build Succeeded

Metrics [docs]

History

jloleysens commented Apr 15, 2021 •

edited

Loading