[ML][Embeddables Rebuild] Migrate Change Point Charts embeddable #182246

darnautov · 2024-05-01T13:48:33Z

Summary

Closes #174963

Decouples UI actions from the embeddable framework
Removes custom Edit action and makes use of the default one
Adds Add action
Migrates Change point detection charts to the react embeddable registry
Replaces the modal for initializing and editing embeddable config with the flyout

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2024-05-02T15:51:38Z

Pinging @elastic/ml-ui (:ml)

…t-refactor

…fix'

x-pack/plugins/aiops/public/embeddable/change_point_chart/initialize_chane_point_controls.ts

x-pack/plugins/aiops/public/embeddable/index.ts

peteharverson

Tested and LGTM. Agree about renaming the embeddable folder and just spotted the typo in the filename.

…3-change-point-refactor

ThomThomson

Great work! It's really nice to see more embeddable conversions coming along!

Left a few nits and comments.

ThomThomson · 2024-05-09T13:30:57Z

...lugins/aiops/public/embeddables/change_point_chart/embeddable_change_point_chart_factory.tsx

+      const blockingError = new BehaviorSubject<Error | undefined>(undefined);
+
+      const dataViews$ = new BehaviorSubject<DataView[] | undefined>([
+        await deps.data.dataViews.get(state.dataViewId),


nit: if you're storing a dataViewId, it should really be stored as a reference. This is so that if a user exports the Dashboard, the data view used in your change point chart is exported as well. You can take a look at examples/embeddable_examples/public/react_embeddables/field_list/field_list_react_embeddable.tsx to see how to accomplish this.

Added in 9323cbf

ThomThomson · 2024-05-09T13:35:39Z

...lugins/aiops/public/embeddables/change_point_chart/embeddable_change_point_chart_factory.tsx

+              api.partitions
+            );
+
+          const reload$ = useMemo(


This is a neat way to translate the fetch pipe into a lastReloadRequestTime. If this ends up being a common pattern, we could make a helper for it - something like useFetch(api) - which would return all of the fetch context as reactive vars.

ThomThomson · 2024-05-09T13:36:36Z

...lugins/aiops/public/embeddables/change_point_chart/embeddable_change_point_chart_factory.tsx

+
+          let embeddingOrigin;
+          if (apiHasExecutionContext(parentApi)) {
+            embeddingOrigin = parentApi.executionContext.type;


I'm wondering why you're using the executionContext type here instead of the parentApi.type?

good question, are there any differences/preferences for that?

We don't have formal best practices around this, but I think the execution context should really only be used for things that are related to data. Like the inspector, telemetry etc. For any other reason - checking if something is rendered on Dashboard etc, the Embeddable parent type should be used.

tagging @nreese for some discussion here.

agree. Use parentApi.type if you need to identify the parent

ThomThomson · 2024-05-09T13:37:41Z

x-pack/plugins/aiops/public/embeddables/change_point_chart/handle_explicit_input.tsx


 export async function resolveEmbeddableChangePointUserInput(
  coreStart: CoreStart,
  pluginStart: AiopsPluginStartDeps,
-  input?: EmbeddableChangePointChartInput
-): Promise<EmbeddableChangePointChartExplicitInput> {
+  input?: ChangePointEmbeddableState


Would it be possible to rename this file? ExplicitInput is a concept that is very much tied to the old system.

renamed in 9323cbf

ThomThomson · 2024-05-09T13:38:09Z

x-pack/plugins/aiops/public/embeddables/change_point_chart/handle_explicit_input.tsx

  const { overlays } = coreStart;

  return new Promise(async (resolve, reject) => {
    try {
-      const modalSession = overlays.openModal(
+      const flyoutSession = overlays.openFlyout(


nit: If you're opening a flyout, you can let the Dashboard know. This allows the Dashboard to blur other panels, disable buttons on the top of the page, and close the flyout if the user navigates away.

You can do this with the following code:

if (apiHasParentApi(api) && tracksOverlays(api.parentApi)) { api.parentApi.openOverlay(overlay, { focusedPanelId: api.uuid }); }

Added in 585cfae

x-pack/plugins/aiops/public/embeddables/index.ts

ThomThomson · 2024-05-09T13:45:16Z

x-pack/plugins/aiops/public/embeddables/change_point_chart/types.ts

+  onRenderComplete?: () => void;
+}>;
+
+export interface ChangePointComponentApi {


Nit: every item you put in here is completely exposed to any other panel or action in the page's context. This isn't necessarily bad, but I'd recommend trying to avoid putting internal state management details on the embeddable's API if possible.

This can be accomplished by passing these things down as props, or by passing a separate stateManager object. You can even pass these things to an editor etc internally without having them on the external API.

ThomThomson · 2024-05-09T13:54:16Z

x-pack/plugins/aiops/public/embeddables/change_point_chart/types.ts

+  maxSeriesToPlot?: number;
+}
+
+export type ChangePointEmbeddableRuntimeState = ChangePointEmbeddableState &


Why are the time range and titles only part of the runtime state? Usually these would be part of the SerializedState because custom panel time ranges and panel titles are saved into the Dashboard.

updated in 9d47fe5

…t-refactor

…fix'

jloleysens

LGTM

peteharverson

Tested latest styling changes (resize of charts, focus for panel when being edited) and LGTM

kibana-ci · 2024-05-13T15:35:22Z

💚 Build Succeeded

Buildkite Build
Commit: 23e4c38

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`aiops`	492	543	+51

Async chunks

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

id	before	after	diff
`aiops`	422.6KB	436.0KB	+13.4KB
`ml`	4.2MB	4.2MB	+4.0KB
total			+17.4KB

Page load bundle

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

id	before	after	diff
`aiops`	7.3KB	7.1KB	-214.0B

Unknown metric groups

async chunk count

id	before	after	diff
`aiops`	18	23	+5

ESLint disabled line counts

id	before	after	diff
`aiops`	25	26	+1

References to deprecated APIs

id	before	after	diff
`aiops`	6	4	-2

Total ESLint disabled count

id	before	after	diff
`aiops`	25	26	+1

History

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

cc @darnautov

darnautov added 2 commits April 30, 2024 10:47

open in ml action

60bc4a2

migrate ui actions, remove custom edit action, react factory wip

34bf293

darnautov added :ml release_note:skip Skip the PR/issue when compiling release notes Team:ML Team label for ML (also use :ml) Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis v8.15.0 labels May 1, 2024

darnautov self-assigned this May 1, 2024

darnautov added 3 commits May 1, 2024 17:53

remove unused code

00b95af

shared component

2933f9e

fix types

8a7328d

darnautov marked this pull request as ready for review May 2, 2024 15:51

darnautov requested a review from a team as a code owner May 2, 2024 15:51

darnautov and others added 4 commits May 3, 2024 09:37

Merge remote-tracking branch 'origin/main' into ml-174963-change-poin…

7e0a69e

…t-refactor

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

49ef10e

…fix'

replace modal with flyout

1d508f3

fix title

65e4344

darnautov requested review from peteharverson and jgowdyelastic May 3, 2024 08:44

peteharverson reviewed May 3, 2024

View reviewed changes

x-pack/plugins/aiops/public/embeddable/change_point_chart/initialize_chane_point_controls.ts Outdated Show resolved Hide resolved

fix i18n

852ab97

jgowdyelastic reviewed May 3, 2024

View reviewed changes

x-pack/plugins/aiops/public/embeddable/index.ts Outdated Show resolved Hide resolved

peteharverson approved these changes May 3, 2024

View reviewed changes

darnautov added 3 commits May 6, 2024 15:34

fix typo

bcef461

rename folder and function

a682cfa

Merge remote-tracking branch 'refs/remotes/origin/main' into ml-17496…

077e52d

…3-change-point-refactor

darnautov requested a review from jgowdyelastic May 6, 2024 14:06

add data-shared-item

389ea7a

ThomThomson approved these changes May 9, 2024

View reviewed changes

darnautov added 2 commits May 13, 2024 10:48

Merge remote-tracking branch 'origin/main' into ml-174963-change-poin…

db3adde

…t-refactor

data view reference

c19f87b

darnautov requested a review from a team as a code owner May 13, 2024 09:59

darnautov and others added 2 commits May 13, 2024 12:03

data view ref, rename file

9323cbf

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

a8ea918

…fix'

jloleysens approved these changes May 13, 2024

View reviewed changes

darnautov added 6 commits May 13, 2024 12:16

track overlay from in the dashboard

585cfae

fix i18n

f4886eb

update state interface, fix ts error

9d47fe5

fix css issues

e893d64

track overlay for swim lane

0350b59

fix track overlay from change point embeddable

4419370

peteharverson approved these changes May 13, 2024

View reviewed changes

darnautov mentioned this pull request May 13, 2024

[ML] Embeddables rebuild #178375

Closed

darnautov enabled auto-merge (squash) May 13, 2024 14:02

set data-test-subj

23e4c38

darnautov merged commit bbd1017 into elastic:main May 13, 2024
18 checks passed

kibanamachine added the backport:skip This commit does not require backporting label May 13, 2024

darnautov deleted the ml-174963-change-point-refactor branch May 13, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML][Embeddables Rebuild] Migrate Change Point Charts embeddable #182246

[ML][Embeddables Rebuild] Migrate Change Point Charts embeddable #182246

darnautov commented May 1, 2024 •

edited

Loading

elasticmachine commented May 2, 2024

peteharverson left a comment

ThomThomson left a comment

ThomThomson May 9, 2024

darnautov May 13, 2024 •

edited

Loading

ThomThomson May 9, 2024

ThomThomson May 9, 2024

darnautov May 13, 2024

ThomThomson May 13, 2024

nreese May 13, 2024

ThomThomson May 9, 2024

darnautov May 13, 2024

ThomThomson May 9, 2024

darnautov May 13, 2024

ThomThomson May 9, 2024

ThomThomson May 9, 2024

darnautov May 13, 2024

jloleysens left a comment

peteharverson left a comment

kibana-ci commented May 13, 2024

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

[ML][Embeddables Rebuild] Migrate Change Point Charts embeddable #182246

[ML][Embeddables Rebuild] Migrate Change Point Charts embeddable #182246

Conversation

darnautov commented May 1, 2024 • edited Loading

Summary

Checklist

elasticmachine commented May 2, 2024

peteharverson left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

kibana-ci commented May 13, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

darnautov commented May 1, 2024 •

edited

Loading

darnautov May 13, 2024 •

edited

Loading