[ML] AIOps: Cleanup context/embeddingOrigin #194442
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
walterra
force-pushed
the
ml-aiops-context-refactor
branch
from
a4b46a3 to
d3a7e1e
Compare
walterra
added
:ml
v9.0.0
Feature:ML/AIOps
|
Pinging @elastic/ml-ui (:ml) |
walterra
added
the
release_note:skip
walterra
commented
Oct 1, 2024
Comment on lines
+24
to
+30
| export const AIOPS_EMBEDDABLE_ORIGIN = { | ||
| CASES: 'cases', | ||
| DASHBOARD: 'dashboard', | ||
| DEFAULT: 'embeddable', | ||
| DISCOVER: 'discover', | ||
| ML_AIOPS_LABS: 'ml_aiops_labs', | ||
| } as const; |
There was a problem hiding this comment.
Originally I wanted to use this to enforce this as a union type when adding embeddingOrigin on the context value. However, this turned out to be tricky for example in Discover where we pass on another value from Discover that's just typed as string. I left this in to be reusable within our own code even when the type is not enforced, it might help with tracking of what kinds of origins we expect to be passed on. Let me know what you think about it.
botelastic
bot
added
ci:project-deploy-observability
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
jgowdyelastic
approved these changes
Oct 2, 2024
walterra
commented
Oct 2, 2024
| deps={services} | ||
| embeddingOrigin="discover" | ||
| appContextValue={ | ||
| { embeddingOrigin: 'discover', ...services } as unknown as AiopsAppContextValue |
There was a problem hiding this comment.
This casting to unknown was previously done inside the component. I was able to remove one layer of adjusting dependencies but it made this necessary on the consuming code. We plan to get rid of the unknown in a follow up.
There was a problem hiding this comment.
Will this be done as an immediate followup? If not, could you leave a comment above the line indicating that it will soon be updated with a link to the followup issue or this PR?
davismcphee
approved these changes
Oct 2, 2024
| deps={services} | ||
| embeddingOrigin="discover" | ||
| appContextValue={ | ||
| { embeddingOrigin: 'discover', ...services } as unknown as AiopsAppContextValue |
There was a problem hiding this comment.
Will this be done as an immediate followup? If not, could you leave a comment above the line indicating that it will soon be updated with a link to the followup issue or this PR?
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 7, 2024
## 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)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Oct 7, 2024
# 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Part of #187772.
We had a mix of passing around
embeddingOriginvia props and context. This PR cleans this up,embeddingOriginis 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
AppDependenciestoAiopsAppContextValue. Originally, this context was used only to pass in dependencies to be used viauseKibana. 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 likeAiopsAppContext.ProvideranduseAiopsAppContext.Checklist