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][Meta] Technical debt and maintenance work for 8.16.0 #187772

Closed
12 tasks done
peteharverson opened this issue Jul 8, 2024 · 2 comments
Closed
12 tasks done

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

peteharverson opened this issue Jul 8, 2024 · 2 comments
Assignees
@walterra @darnautov @alvarezmelissa87 @peteharverson @jgowdyelastic
Labels
epic Meta :ml refactoring technical debt Improvement of the software architecture and operational architecture v8.16.0

Comments

@peteharverson
Copy link
Contributor

peteharverson commented Jul 8, 2024 •
edited
Loading

This issue lists items from the dependency cache issue as well as the migration of js files to TypeScript issue as well as other tech debt to be resolved as part of 8.16.

Misc
Preview Give feedback
  1. [DataViews] Evaluate switching ML alert rules to make use of DataViewLazy #173996
    :ml Feature:Data Views Team:DataDiscovery impact:medium loe:needs-research technical debt v8.16.0
    darnautov
  2. [ML] CSS Deprecation Issue for Team: ml-ui, Found in file: help_popover.scss #190926
    :ml Team:ML good first issue v8.16.0
    rbrtj
  3. [ML] Move FieldStatsFlyout, parseInterval, validators to packages. #193657
    :ml Feature:Transforms backport:prev-minor release_note:skip v8.16.0 v9.0.0
    walterra
  4. [ML] AIOps: Cleanup context/embeddingOrigin #194442
    :ml Feature:ML/AIOps Team:obs-ux-management backport:version ci:project-deploy-observability release_note:skip v8.16.0 v9.0.0
    walterra
  5. [ML] AIOps: Use package instead of context for using field stats flyout #194517
    :ml Feature:ML/AIOps backport:version release_note:skip v8.16.0 v9.0.0
    walterra
  6. [ML] Remove legacy scss overwrites Single Metric Viewer #195259
    :ml Team:ML backport:version release_note:skip v8.16.0 v9.0.0
    rbrtj

Dependency cache #153476
Preview Give feedback
  1. [ML] Remove dependency cache. #189729
    :ml backport:skip release_note:skip v8.16.0
    walterra
  2. [ML] Remove static api service #153477
    :ml Meta refactoring technical debt v8.16.0
  3. [ML] Renaming ML API service #191742
    :ml backport:skip chore non-issue release_note:skip v8.16.0
    jgowdyelastic
  4. [ML] Job service refactor #191724
    :ml Feature:Anomaly Detection backport:skip release_note:skip v8.16.0
    jgowdyelastic

JS files that need to be migrated to TypeScript #153894
Preview Give feedback

Migrate tests using enzyme to use @testing-library/react. #153288
Preview Give feedback
  1. [ML] Remove dependency cache. #189729
    :ml backport:skip release_note:skip v8.16.0
    walterra
@peteharverson peteharverson added Meta :ml refactoring technical debt Improvement of the software architecture and operational architecture epic v8.16.0 labels Jul 8, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson peteharverson mentioned this issue Jul 8, 2024
Closed
@walterra walterra mentioned this issue Aug 8, 2024
Merged
3 tasks
walterra added a commit that referenced this issue Aug 29, 2024
@walterra
[ML] Remove dependency cache. (#189729)
38f4aa0 
## 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

- [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
- [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)
This was referenced Oct 1, 2024
Merged
Merged
walterra added a commit that referenced this issue Oct 2, 2024
@walterra
[ML] AIOps: Use package instead of context for using field stats flyo…
eebfba4 
…ut (#194517)

## Summary

Part of #187772.
Follow up to #193657.

The previous PR #193657 moved `FieldStatsFlyout` to a package, the
`aiops` plugin didn't make full use of that refactor by still passing in
the flyout into the app context.

### Checklist

- [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)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 2, 2024
@walterra
[ML] AIOps: Use package instead of context for using field stats flyo…
55a1b33 
…ut (elastic#194517)

## Summary

Part of elastic#187772.
Follow up to elastic#193657.

The previous PR elastic#193657 moved `FieldStatsFlyout` to a package, the
`aiops` plugin didn't make full use of that refactor by still passing in
the flyout into the app context.

### Checklist

- [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)

(cherry picked from commit eebfba4)
walterra added a commit that referenced this issue Oct 7, 2024
@walterra
[ML] AIOps: Cleanup context/embeddingOrigin (#194442)
ae36dd5 
## Summary

Part of #187772.

We had a mix of passing around `embeddingOrigin` via props and context.
This PR cleans this up, `embeddingOrigin` is now be required to be
passed in on the outer most component and will then be used internally
via context only.

The PR also renames references to `AppDependencies` to
`AiopsAppContextValue`. Originally, this context was used only to pass
in dependencies to be used via `useKibana`. Over time this changed a bit
and we started passing in other non-changing values, the naming change
now reflects that more properly and brings the name in line with the
other context related vars like `AiopsAppContext.Provider` and
`useAiopsAppContext`.


### Checklist

- [ ] [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)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 7, 2024
@walterra
[ML] AIOps: Cleanup context/embeddingOrigin (elastic#194442)
178992d 
## Summary

Part of elastic#187772.

We had a mix of passing around `embeddingOrigin` via props and context.
This PR cleans this up, `embeddingOrigin` is now be required to be
passed in on the outer most component and will then be used internally
via context only.

The PR also renames references to `AppDependencies` to
`AiopsAppContextValue`. Originally, this context was used only to pass
in dependencies to be used via `useKibana`. Over time this changed a bit
and we started passing in other non-changing values, the naming change
now reflects that more properly and brings the name in line with the
other context related vars like `AiopsAppContext.Provider` and
`useAiopsAppContext`.

### Checklist

- [ ] [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)

(cherry picked from commit ae36dd5)
kibanamachine added a commit that referenced this issue Oct 7, 2024
@kibanamachine @walterra
[8.x] [ML] AIOps: Cleanup context/embeddingOrigin (#194442) (#195284)
07c493c 
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ML] AIOps: Cleanup context/embeddingOrigin
(#194442)](#194442)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Walter
Rafelsberger","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T15:32:18Z","message":"[ML]
AIOps: Cleanup context/embeddingOrigin (#194442)\n\n##
Summary\r\n\r\nPart of #187772.\r\n\r\nWe had a mix of passing around
`embeddingOrigin` via props and context.\r\nThis PR cleans this up,
`embeddingOrigin` is now be required to be\r\npassed in on the outer
most component and will then be used internally\r\nvia context
only.\r\n\r\nThe PR also renames references to `AppDependencies`
to\r\n`AiopsAppContextValue`. Originally, this context was used only to
pass\r\nin dependencies to be used via `useKibana`. Over time this
changed a bit\r\nand we started passing in other non-changing values,
the naming change\r\nnow reflects that more properly and brings the name
in line with the\r\nother context related vars like
`AiopsAppContext.Provider` and\r\n`useAiopsAppContext`.\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ae36dd5bf2713cafc2e5eaa73629612686842c1a","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","v9.0.0","Feature:ML/AIOps","ci:project-deploy-observability","Team:obs-ux-management","v8.16.0","backport:version"],"title":"[ML]
AIOps: Cleanup
context/embeddingOrigin","number":194442,"url":"https://github.com/elastic/kibana/pull/194442","mergeCommit":{"message":"[ML]
AIOps: Cleanup context/embeddingOrigin (#194442)\n\n##
Summary\r\n\r\nPart of #187772.\r\n\r\nWe had a mix of passing around
`embeddingOrigin` via props and context.\r\nThis PR cleans this up,
`embeddingOrigin` is now be required to be\r\npassed in on the outer
most component and will then be used internally\r\nvia context
only.\r\n\r\nThe PR also renames references to `AppDependencies`
to\r\n`AiopsAppContextValue`. Originally, this context was used only to
pass\r\nin dependencies to be used via `useKibana`. Over time this
changed a bit\r\nand we started passing in other non-changing values,
the naming change\r\nnow reflects that more properly and brings the name
in line with the\r\nother context related vars like
`AiopsAppContext.Provider` and\r\n`useAiopsAppContext`.\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ae36dd5bf2713cafc2e5eaa73629612686842c1a"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194442","number":194442,"mergeCommit":{"message":"[ML]
AIOps: Cleanup context/embeddingOrigin (#194442)\n\n##
Summary\r\n\r\nPart of #187772.\r\n\r\nWe had a mix of passing around
`embeddingOrigin` via props and context.\r\nThis PR cleans this up,
`embeddingOrigin` is now be required to be\r\npassed in on the outer
most component and will then be used internally\r\nvia context
only.\r\n\r\nThe PR also renames references to `AppDependencies`
to\r\n`AiopsAppContextValue`. Originally, this context was used only to
pass\r\nin dependencies to be used via `useKibana`. Over time this
changed a bit\r\nand we started passing in other non-changing values,
the naming change\r\nnow reflects that more properly and brings the name
in line with the\r\nother context related vars like
`AiopsAppContext.Provider` and\r\n`useAiopsAppContext`.\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ae36dd5bf2713cafc2e5eaa73629612686842c1a"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Walter Rafelsberger <[email protected]>
@peteharverson
Copy link
Contributor Author

Closing, all items planned for 8.16 have been completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@walterra walterra

@darnautov darnautov

@alvarezmelissa87 alvarezmelissa87

@peteharverson peteharverson

@jgowdyelastic jgowdyelastic

Labels
epic Meta :ml refactoring technical debt Improvement of the software architecture and operational architecture v8.16.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@walterra @darnautov @alvarezmelissa87 @peteharverson @elasticmachine @jgowdyelastic