-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Switching to new es client #74965
[ML] Switching to new es client #74965
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
merge conflict between base and head |
retest |
@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.
LGTM! Would be great to centralize the ES client mocks in a follow-up 🙂
@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.
Uptime changes LGTM.
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.
APM changes LGTM
await ml.jobServiceProvider(scopedClusterClient, ({} as unknown) as KibanaRequest) | ||
).jobsSummary([machineLearningJobId]); | ||
const fakeRequest = {} as KibanaRequest; | ||
const summaryJobs = await ml |
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.
This looks to break ML Rules: when an ML Rule executes, summaryJobs
is now undefined
, while on master it is MlSummaryJob[]
.
I apologize for not having tests that exercise this code: we're in the process of moving to the FTR where we can leverage ML setup helpers, but until then we do not have jobs config/anomalies data to test against.
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.
fixed in 18ccad3
@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.
Seems to function as expected for the infra
plugin. I only have one question below about the log verbosity.
.isFullLicense() | ||
.hasMlCapabilities(['canGetJobs']) | ||
.ok(async ({ scopedClient }) => { | ||
const { body } = await scopedClient.asInternalUser.ml.getJobs<{ |
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.
Is it possible that this logs errors to the Kibana log when querying for a non-existent job, which it didn't before? Is there a less noisy way of checking for a job's existence?
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.
yes it appears this is a change introduced with using the new js client. Kibana is logging all js client errors to its log.
I am able to disable the log message by passing { ignore: [404] }
as an option to getJobs
.
I don't know whether this is something we should be doing by default or perhaps exposing this option to consumers of our shared functions.
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.
For an API that returns a collection I would expect the result set to just not contain the items it couldn't find. 404s/"not found" exceptions mainly make sense for single-resource APIs IMHO.
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.
Yes, that does make sense, unfortunately this is a basic wrapper around the elasticsearch endpoint, and so behaves in the exact same way.
https://www.elastic.co/guide/en/elasticsearch/reference/master/ml-get-job.html
💚 Build SucceededBuild metricsasync chunks size
distributable file count
History
To update your PR or re-run it, just comment with: |
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.
API integration test changes LGTM
* [ML] Switching to new es client * further conversions * fixing tests * updating responses * test commit * refactoring shared services to removed context parameter * removing last scoped clients * removing ml js client * udating file data viz errors * fixing jest tests * fixing types after merge with master * error response changes * adding default sizes to some requests * adding custom error types for license checks * tidying up shared function checks * removing test data * removing need for DummyKibanaRequest * updating comment * fixing functional api tests * removing comments * fixing types after master merge * throw error rather than return it * removing placeholder error * changes based on review comments * fixing types after merge with master * fixing missing return Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* [ML] Switching to new es client * further conversions * fixing tests * updating responses * test commit * refactoring shared services to removed context parameter * removing last scoped clients * removing ml js client * udating file data viz errors * fixing jest tests * fixing types after merge with master * error response changes * adding default sizes to some requests * adding custom error types for license checks * tidying up shared function checks * removing test data * removing need for DummyKibanaRequest * updating comment * fixing functional api tests * removing comments * fixing types after master merge * throw error rather than return it * removing placeholder error * changes based on review comments * fixing types after merge with master * fixing missing return Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
A regression was introduced in elastic#74965 that caused an error to be thrown while collecting telemetry on ML jobs. Because such exceptions are caught and we degrade to zeroing out those counts, this one was not caught until manual testing of telemetry.
A regression was introduced in #74965 that caused an error to be thrown while collecting telemetry on ML jobs. Because such exceptions are caught and we degrade to zeroing out those counts, this one was not caught until manual testing of telemetry.
A regression was introduced in elastic#74965 that caused an error to be thrown while collecting telemetry on ML jobs. Because such exceptions are caught and we degrade to zeroing out those counts, this one was not caught until manual testing of telemetry.
A regression was introduced in #74965 that caused an error to be thrown while collecting telemetry on ML jobs. Because such exceptions are caught and we degrade to zeroing out those counts, this one was not caught until manual testing of telemetry.
A regression was introduced in #74965 that caused an error to be thrown while collecting telemetry on ML jobs. Because such exceptions are caught and we degrade to zeroing out those counts, this one was not caught until manual testing of telemetry.
Changes:
legacyClient
body
which contains the response payload or error object.request
object passed to the shared services providers.DummyKibanaRequest
. Instead we check that the request is a real instance ofKibanaRequest
message
now contains less information. So we'll need to dig through the raw error contained in theattributes
object.More information about the new es client can be found here https://github.com/elastic/kibana/blob/master/src/core/MIGRATION_EXAMPLES.md#elasticsearch-client
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately