Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] Job service refactor #191724

Merged
merged 10 commits into from
Sep 10, 2024
Merged

[ML] Job service refactor #191724

merged 10 commits into from
Sep 10, 2024

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Aug 29, 2024 •
edited
Loading

Follow on from dependancy cache removal #189729

Moving and removing as much as possible out of the JobService class.

  • Moving the job cloning functions to a separate class as they do not require the same dependancies as JobService
  • Removing api basic wrapper functions like saveNewJob, openJob, forceStartDatafeeds etc
  • Removes toast dependency. This might be controversial, but I think it is unnecessary for these basic job/datafeed loading functions to show a toast if they fail. If we cannot load the basic list of jobs then there is something else very wrong with the system. We also throw the errors, so they will not be lost. It should be up to the calling function to display a toast. Removing this dependency cleans up the code quite a lot.
  • The JobCreator classes no longer use JobService

@jgowdyelastic
Copy link
Member Author

/ci

@jgowdyelastic
Copy link
Member Author

/ci

@jgowdyelastic jgowdyelastic self-assigned this Sep 5, 2024
@jgowdyelastic jgowdyelastic added :ml release_note:skip Skip the PR/issue when compiling release notes v8.16.0 Feature:Anomaly Detection ML anomaly detection labels Sep 5, 2024
@jgowdyelastic jgowdyelastic marked this pull request as ready for review September 5, 2024 14:05
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner September 5, 2024 14:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I gave a variety of anomaly detection job functionality a test (creating, cloning, starting, stopping, editing; in the default and another space), and couldn't spot any regressions.

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts
@@ -241,7 +238,7 @@ export class JobCreator {
public set jobId(jobId: JobId) {
this._job_config.job_id = jobId;
this._datafeed_config.job_id = jobId;
this._datafeed_config.datafeed_id = `datafeed-${jobId}`;
this._datafeed_config.datafeed_id = createDatafeedId(jobId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to the changes here as also happens on main, but hit this error when creating a job off the Patterns tab in Discover and then switching to the job wizard:

Screenshot 2024-09-09 at 10 17 42 Screenshot 2024-09-09 at 10 35 58

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've raised a separate PR to fix this

walterra
walterra approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just added a question about naming tempJobCloningObjects.

x-pack/plugins/ml/public/application/services/job_cloning_service.ts Outdated
import { isPopulatedObject } from '@kbn/ml-is-populated-object';
import type { JobCreator } from '../jobs/new_job/common/job_creator/job_creator';

interface TempJobCloningObjects {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Wondering why this is plural and not singular TempJobCloningObject? Maybe replace with TempJobCloningData?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, TempJobCloningData is better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 4a13c21

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #31 / discover/context_awareness extension getDocViewer data view mode should render logs overview tab for logs data source
  • [job] [logs] FTR Configs #78 / Rules Management - Prebuilt Rules - Large Prebuilt Rules Package @ess @serverless @skipInServerlessMKI install_large_prebuilt_rules_package should install a package containing 15000 prebuilt rules without crashing

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 2024 2026 +2

Async chunks

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

id before after diff
ml 4.6MB 4.6MB -72.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 78.7KB 78.7KB +48.0B
Unknown metric groups

async chunk count

id before after diff
ml 108 110 +2

ESLint disabled line counts

id before after diff
ml 566 563 -3

Total ESLint disabled count

id before after diff
ml 569 566 -3

History

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

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic merged commit 2d40fa3 into elastic:main Sep 10, 2024
22 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 10, 2024
@jgowdyelastic jgowdyelastic deleted the job-service-refactor branch September 10, 2024 16:58
@szaffarano szaffarano mentioned this pull request Sep 13, 2024
Merged
1 task
@peteharverson peteharverson mentioned this pull request Sep 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra walterra approved these changes

Assignees

@jgowdyelastic jgowdyelastic

Labels
backport:skip This commit does not require backporting Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jgowdyelastic @elasticmachine @kibana-ci @walterra @peteharverson @kibanamachine