-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Watcher] Migrate to new ES client #97260
[Watcher] Migrate to new ES client #97260
Conversation
IScopedClusterClient and from "isEsError" to "handleEsError"
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
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
): Promise<any> { | ||
const newHits = get(searchResuls, 'hits.hits', []); | ||
const scrollId = get(searchResuls, '_scroll_id'); | ||
searchResults: ScrollResponse<unknown>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice typo catch!
@@ -33,9 +29,9 @@ describe('fetch_all_from_scroll', () => { | |||
}); | |||
}); | |||
|
|||
it('should not call callWithRequest', () => { | |||
it('should not call asCurrentUser.scroll', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how these tests became clearer as a result of this migration!
return response.aggregations.indices.buckets.map((bucket: any) => bucket.key); | ||
}); | ||
}); | ||
return indices.buckets ? indices.buckets.map((bucket) => bucket.key) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
{ | ||
index: indexes, | ||
fields: ['*'], | ||
allow_no_indices: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the ignoreUnavailable
param was omitted. Was that deliberate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope this was a mistake!
// 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 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look into how to test this, either in this handler or in the other route handlers that contain this TODO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
{ | ||
index, | ||
body, | ||
allow_no_indices: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the decision to omit ignoreUnavailable
was deliberate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, will re-add
}, | ||
{ ignore: [404] } | ||
) | ||
.then(({ body: response }) => fetchAllFromScroll(response, dataClient)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: why not pass body
as-is instead of renaming it?
.then(({ body }) => fetchAllFromScroll(body, dataClient));
@elasticmachine merge upstream |
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 |
I always leave this for last for some reason 😅 , seems like to should be the first order of business. |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* 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>
* 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>
Summary
See #73973
How to test
.kibana
index so that the alert will fire