[ML] Remove dependency cache. #189729

walterra · 2024-08-01T15:55:15Z

Summary

Fixes #153477.
Fixes #153476.
Part of #187772 (technical debt).
Part of #153288 (migrate enzyme tests to react-testing-lib).

Removes dependency cache. The major culprit making this PR large and not easy to split is that getHttp() from the dependency cache was used throughout the code base for services like mlJobService and ml/mlApiServices which then themselves were directly imported and not part of React component lifecycles.

For functional components this means mostly migrating to hooks that allow accessing services.
We still have a bit of a mix of usage of withKibana and context for class based React components. This was not consolidated in this PR, I took what's there and adjusted how services get used. These components access services via this.props.kibana.services.* or this.context.services.*.
Functions no longer access the global services provided via dependency cache but were updated to receive services via arguments.
Stateful services like mlJobService are exposed now via a factory that makes sure the service gets instantiated only once.
Some tests where the mocks needed quite some refactoring were ported to react-testing-lib. They no longer make use of snapshots or call component methods which should be considered implementation details.
We have a mix of usage of the plain toasts via useMlKibana and our own toastNotificationServices that wraps toasts. I didn't consolidate this in this PR but used what's available for the given code.
For class based components, service initializations were moved from componentDidMount() to constructor() where I spotted it.
We have a bit of a mix of naming: ml, mlApiServices, useMlApiContext() for the same thing. I didn't consolidate the naming in this PR, to avoid making this PR even larger. This can be done in a follow up, once this PR is in this should be more straightforward and less risky.
Turns out explorer_chart_config_builder.js is no longer used anywhere so I deleted it.
jobs/jobs_list/components/utils.d.ts was missing some definitions, tried to fix them.
Moved stashJobForCloning to be a method of mlJobService.
The MetricSelector component was an exact copy besides the i18n label, consolidated that so anomaly detection wizards use the same component.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
This was checked for breaking API changes and was labeled appropriately

alvarezmelissa87 · 2024-08-21T20:14:31Z

.../data_frame_analytics/pages/analytics_creation/components/details_step/details_step_form.tsx

@@ -43,6 +42,7 @@ export const DetailsStepForm: FC<CreateAnalyticsStepProps> = ({
  const {
    services: { docLinks, notifications },
  } = useMlKibana();
+  const ml = useMlApiContext();


alvarezmelissa87 · 2024-08-21T20:17:24Z

Awesome work getting rid of the dependency cache finally! Functionality for DFA looks good - just added a few comments to the code.

peteharverson

LGTM after latest edits for the areas I tested:

Embeddables
Single Metric Viewer
Overview page
Notifications
Memory Usage

alvarezmelissa87

LGTM ⚡

jgowdyelastic · 2024-08-28T13:14:24Z

x-pack/plugins/ml/public/embeddables/job_creation/aiops/flyout/create_job.tsx

      ),
-
-    [dashboardService, data, mlApiServices, uiSettings]
+    // eslint-disable-next-line react-hooks/exhaustive-deps


from my testing this comment isn't needed and the dependency list can be correct.

Updated in 393abda.

jgowdyelastic · 2024-08-28T14:15:22Z

x-pack/plugins/ml/public/application/services/http_service.ts

 /**
 * ML Http Service
 */
 export class HttpService {
  public getLoadingCount$: Observable<number>;

-  constructor(private httpStart: HttpStart) {
-    this.getLoadingCount$ = httpStart.getLoadingCount$();
+  constructor(private httpSetup: HttpSetup) {


This could stay as HttpStart, they are the same it seems, but we use the one from coreStart which is HttpStart

Updated in Updated in 393abda.

jgowdyelastic · 2024-08-28T15:48:51Z

...ck/plugins/ml/public/application/jobs/new_job/common/job_creator/multi_metric_job_creator.ts

@@ -90,7 +100,13 @@ export class MultiMetricJobCreator extends JobCreator {
    this._overrideConfigs(job, datafeed);
    this.createdBy = CREATED_BY_LABEL.MULTI_METRIC;
    this._sparseData = isSparseDataJob(job, datafeed);
-    const detectors = getRichDetectors(job, datafeed, this.additionalFields, false);
+    const detectors = getRichDetectors(


I think getRichDetectors (and getFieldByIdFactory) could be moved to be a member of the base class. It then wouldn't need newJobCapsService to be passed to it.

I can do this in a follow up, if you don't want to complicate this PR any further.

Yes, let's do this in follow ups please 😅 .

jgowdyelastic · 2024-08-28T15:55:28Z

This a great bit of refactoring, thanks for doing it.

I'm going to follow it up with some further changes to move stashJobForCloning out of mlJobService. I think having it inside there complicates things. mlJobService's dependencies are unnecessary for the stashing of the job and moving it into it's own file or service will clean up all the places where we need to stash a job for cloning.
I don't think we need to make these changes in this PR as it'll be good to just get this merged.

…into ml-rip-dependency-cache

jgowdyelastic

LGTM

kibana-ci · 2024-08-28T18:56:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 9ed78d7

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	2026	2025	-1

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	+20.7KB

Page load bundle

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

id	before	after	diff
`ml`	80.5KB	78.7KB	-1.8KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`ml`	550	566	+16

Total ESLint disabled count

id	before	after	diff
`ml`	553	569	+16

History

💛 Build #230070 was flaky 2905d30
💚 Build #229601 succeeded f8bcd58
💚 Build #228863 succeeded f7b99c2
💛 Build #226478 was flaky 9e35cfb
💔 Build #226446 failed 88f54e0d19353bc6068fc23342fd3eb99ed2120e
💔 Build #226425 failed 3605942f7e78cc27d3e07563232fc67d2c74c44a

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

cc @walterra

darnautov

LGTM

Addresses the comment from the description of this [PR](#189729): `We have a bit of a mix of naming: ml, mlApiServices, useMlApiContext() for the same thing` Renames: `useMlApiContext` -> `useMlApi` `ml` -> `mlApi` `mlApiServices` -> `mlApi` `MlApiServices ` -> `MlApi` E.g. we can now use `const mlApi = useMlApi();` Rather than: `const ml = useMlApiContext();` or `const mlServices = useMlApiContext();` or ``` const { services: { mlServices: { mlApiServices }, }, } = useMlKibana(); ```

…asts table fix (#192000) ## Summary Fix for: [#191936](#191936) Fixes navigation from the Forecasts & Annotations table to the Single Metric Viewer. Added functional tests. Tested on 8.15 - issue doesn't exist, it was probably introduced in [#189729](#189729) ### Checklist - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

## Summary Upgrades `@testing-library/user-event` to `^14.5.2`. See the release notes for `v14` for breaking changes: https://github.com/testing-library/user-event/releases/tag/v14.0.0 I was facing an [issue](testing-library/user-event#662) with `v13.5.0` with `userEvent.click()` in a PR (#189729) and was able to verify that `v14.4.3` onwards fixes it so I decided to update that package. What a rabbit hole 😅 ! - In `user-event` `v14` events return a promise, so this PR updates usage of the likes of `userEvent.click` with `await userEvent.click`. Regex to search for `userEvent` calls that miss `await` except `.setup`: `(?<!await\s)userEvent\.(?!setup\b)` - The way to handle pointer events needed changing from `, undefined, { skipPointerEventsCheck: true });` to `, { pointerEventsCheck: 0 });`. - I tried a bit to do the refactor with codemods, but there were quite some edge cases so it ended up being done manually. - I looked into all failing tests and tried my best to update them, but for some of them I lacked the context to make them work again. If you're a code owner and find a skipped test in this PR please give it a try to fix and push in this PR or let me know if it's fine for you to fix in follow ups. List of files where I had to skip tests (`git diff main...HEAD -G'\.skip' --name-only`): ### `packages/kbn-dom-drag-drop` - `packages/kbn-dom-drag-drop/src/droppable.test.tsx` ### `x-pack/plugins/cases` - `x-pack/plugins/cases/public/components/templates/form.test.tsx` - `x-pack/plugins/cases/public/components/user_actions/user_actions_list.test.tsx` ### `x-pack/plugins/cloud_security_posture` - `x-pack/plugins/cloud_security_posture/public/components/fleet_extensions/policy_template_form.test.tsx` ### `x-pack/plugins/lens` - `x-pack/plugins/lens/public/datasources/form_based/dimension_panel/format_selector.test.tsx` ### `x-pack/plugins/observability_solution` - `x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitor_add_edit/fields/request_body_field.test.tsx` ### `x-pack/plugins/security_solution` - `x-pack/plugins/security_solution/public/management/components/console/components/command_input/integration_tests/command_input.test.tsx` - `x-pack/plugins/security_solution/public/management/components/endpoint_responder/command_render_components/integration_tests/kill_process_action.test.tsx` - `x-pack/plugins/security_solution/public/management/components/endpoint_responder/command_render_components/integration_tests/release_action.test.tsx` - `x-pack/plugins/security_solution/public/management/components/endpoint_responder/command_render_components/integration_tests/status_action.test.tsx` - `x-pack/plugins/security_solution/public/management/components/endpoint_responder/command_render_components/integration_tests/upload_action.test.tsx` - `x-pack/plugins/security_solution/public/management/components/endpoint_response_actions_list/integration_tests/response_actions_log.test.tsx` - `x-pack/plugins/security_solution/public/management/pages/event_filters/view/components/event_filters_flyout.test.tsx` - `x-pack/plugins/security_solution/public/management/pages/response_actions/view/response_actions_list_page.test.tsx` ---- I plan to do a talk on Kibana Demo Days to walk through some of the breaking changes and learnings. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

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`

walterra self-assigned this Aug 1, 2024

walterra force-pushed the ml-rip-dependency-cache branch 3 times, most recently from ebcb88c to 17aa860 Compare August 6, 2024 08:47

walterra mentioned this pull request Aug 6, 2024

Upgrade @testing-library/user-event to latest ^14.5.2 #189949

Merged

3 tasks

elastic deleted a comment from kibana-ci Aug 6, 2024

walterra force-pushed the ml-rip-dependency-cache branch 2 times, most recently from 4114a27 to 17d4928 Compare August 8, 2024 06:31

walterra changed the title ~~[ML] Refactor ml and other services away from dependency cache.~~ [ML] Remove dependency cache. Aug 8, 2024

This was referenced Aug 8, 2024

[ML][Meta] Technical debt and maintenance work for 8.16.0 #187772

Closed

[ML] Migrate tests using enzyme to use @testing-library/react. #153288

Open

walterra added :ml release_note:skip Skip the PR/issue when compiling release notes v8.16.0 labels Aug 8, 2024

walterra added 16 commits August 8, 2024 12:23

refactor away from dependency cache

3fe0365

cleanup

a333e80

fix jest test for filter_lists

8e581cf

fix jest test for calendars_list

6ca6cf2

migrate jest test for calendars_list from enzyme to react-testing-lib

6a74c64

fix jest test for settings

11c6439

fix jest test for model_memory_status

b095241

fix jest test for annotation_flyout

219bcfe

fix jest test for rule_action_panel

a868628

migrate jest test for rule_action_panel from enzyme to react-testing-lib

a667d71

fix jest test for delete_action_name

9fde7d6

import fixes

b3c8061

remove unused code

e3a4838

context fixes

3482e76

more context fixes

b8ecaee

remove i18n from dependency cache

d3ae0ed

alvarezmelissa87 reviewed Aug 21, 2024

View reviewed changes

walterra added 3 commits August 22, 2024 11:02

Merge branch 'main' into ml-rip-dependency-cache

391e2cf

Merge branch 'main' into ml-rip-dependency-cache

f8bcd58

Merge branch 'main' into ml-rip-dependency-cache

2905d30

peteharverson approved these changes Aug 27, 2024

View reviewed changes

alvarezmelissa87 approved these changes Aug 27, 2024

View reviewed changes

jgowdyelastic reviewed Aug 28, 2024

View reviewed changes

walterra added 2 commits August 28, 2024 18:54

review feedback

393abda

Merge branch 'ml-rip-dependency-cache' of github.com:walterra/kibana …

9ed78d7

…into ml-rip-dependency-cache

jgowdyelastic approved these changes Aug 28, 2024

View reviewed changes

darnautov approved these changes Aug 29, 2024

View reviewed changes

walterra merged commit 38f4aa0 into elastic:main Aug 29, 2024
21 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Aug 29, 2024

walterra deleted the ml-rip-dependency-cache branch August 29, 2024 09:35

This was referenced Aug 29, 2024

[ML] Job service refactor #191724

Merged

[ML] Renaming ML API service #191742

Merged

rbrtj mentioned this pull request Sep 3, 2024

[ML] Fixes links to the Single Metric Viewer from the Annotations and Forecasts tables #192000

Merged

1 task

peteharverson mentioned this pull request Sep 12, 2024

[ML] Anomaly detection: Hide Top influencers panel for jobs without influencers #192679

Closed

peteharverson mentioned this pull request Oct 23, 2024

[ML] Increase Test Coverage 8.17.0 #197420

Closed

8 tasks

peteharverson mentioned this pull request Nov 26, 2024

[ML] Increase Test Coverage 8.18.0 #201813

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Remove dependency cache. #189729

[ML] Remove dependency cache. #189729

walterra commented Aug 1, 2024 •

edited

Loading

alvarezmelissa87 Aug 21, 2024

alvarezmelissa87 commented Aug 21, 2024

peteharverson left a comment

alvarezmelissa87 left a comment

jgowdyelastic Aug 28, 2024

walterra Aug 28, 2024

jgowdyelastic Aug 28, 2024

walterra Aug 28, 2024

jgowdyelastic Aug 28, 2024

walterra Aug 28, 2024

jgowdyelastic commented Aug 28, 2024

jgowdyelastic left a comment

kibana-ci commented Aug 28, 2024

ESLint disabled line counts

Total ESLint disabled count

darnautov left a comment

[ML] Remove dependency cache. #189729

[ML] Remove dependency cache. #189729

Conversation

walterra commented Aug 1, 2024 • edited Loading

Summary

Checklist

alvarezmelissa87 Aug 21, 2024

Choose a reason for hiding this comment

alvarezmelissa87 commented Aug 21, 2024

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

jgowdyelastic Aug 28, 2024

Choose a reason for hiding this comment

walterra Aug 28, 2024

Choose a reason for hiding this comment

jgowdyelastic Aug 28, 2024

Choose a reason for hiding this comment

walterra Aug 28, 2024

Choose a reason for hiding this comment

jgowdyelastic Aug 28, 2024

Choose a reason for hiding this comment

walterra Aug 28, 2024

Choose a reason for hiding this comment

jgowdyelastic commented Aug 28, 2024

jgowdyelastic left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 28, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

darnautov left a comment

Choose a reason for hiding this comment

walterra commented Aug 1, 2024 •

edited

Loading