[ML][Embeddables Rebuild] Migrate Anomaly Chart #183456

qn895 · 2024-05-14T23:58:02Z

Summary

This PR addresses #174966 and migrates Anomaly charts embeddable to the new React architecture.

Adding a new Anomaly chart panel in Dashboard:

Screen.Recording.2024-05-20.at.11.20.14.mov

Saving dashboard with settings like anomaly threshold persisted

Adding a new Anomaly chart to a case

Changes include:

Migrate embeddable framework to React rebuild
Detach UI create chart action
Remove Edit UI action
Update case attachment for anomaly charts to no longer need embeddable
Changes also include a refactor in how the Anomaly charts update cursor sync data. Previously, when cursor is updated, the whole chart will re-render. The new refactor optimizes so that the chart is rendered only when needed, and the cursor line will update independently.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…bed-migrate # Conflicts: # x-pack/plugins/ml/public/ui_actions/edit_single_metric_viewer_panel_action.tsx # x-pack/plugins/ml/public/ui_actions/index.ts

qn895 · 2024-05-17T01:18:02Z

/ci

qn895 · 2024-05-17T03:50:25Z

/ci

qn895 · 2024-05-17T17:05:17Z

/ci

… job flyout

qn895 · 2024-05-21T19:33:59Z

Thanks @peteharverson and @alvarezmelissa87 for testing. I've fixed the 'unsaved changes', chart text overflowing/cutting off, and timeout warning in the data report in the latest changes.

Screen.Recording.2024-05-21.at.14.32.53.mov

qn895 · 2024-05-21T20:06:19Z

x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_anomalies_container.tsx

@@ -62,7 +62,9 @@ export const ExplorerAnomaliesContainer: FC<ExplorerAnomaliesContainerProps> = (
  timeRange,
 }) => {
  return (
-    <>
+    // TODO: Remove data-shared-item and data-rendering-count as part of https://github.com/elastic/kibana/issues/179376


Note for reviewers: Leaving this here as todo for future when referenced Kibana issue is closed

ThomThomson · 2024-05-21T21:25:13Z

x-pack/plugins/ml/public/embeddables/anomaly_charts/anomaly_charts_embeddable_factory.tsx

+  const factory: ReactEmbeddableFactory<
+    AnomalyChartsEmbeddableState,
+    AnomalyChartsEmbeddableApi,
+    AnomalyChartsEmbeddableState


nit: You don't need this third type argument. The runtime state type defaults to the same as the serialized state.

Thanks for the hint. Updated here 1ed16a4

ThomThomson · 2024-05-21T21:26:28Z

x-pack/plugins/ml/public/embeddables/anomaly_charts/anomaly_charts_embeddable_factory.tsx

+        }
+      );
+
+      const appliedTimeRange$: Observable<TimeRange | undefined> = combineLatest({


Yes this is very true. Ideally fetch$ can simplify this code quite a lot!

alvarezmelissa87 · 2024-05-21T21:37:50Z

Confirmed everything is working as expected now except the text overflow only in dashboard and only when it's multiple charts.

The pdf/png export appears to be working but, as with other embeddables, seeing the completed with issues warning.

This might require more digging and should be okay to do in a follow up as it's an existing issue.

…bed-migrate

…sole

…bed-migrate

peteharverson

Tested latest change and LGTM.

darnautov · 2024-05-23T09:40:40Z

x-pack/plugins/ml/public/application/explorer/anomaly_context_menu.tsx

  const isMaxSeriesToPlotValid =
-    maxSeriesToPlot >= 1 && maxSeriesToPlot <= MAX_ANOMALY_CHARTS_ALLOWED;
+    typeof maxSeriesToPlot === 'number' &&
+    maxSeriesToPlot >= 1 &&
+    maxSeriesToPlot <= MAX_ANOMALY_CHARTS_ALLOWED;


did you consider using numberValidator?

x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_chart_distribution.js

x-pack/plugins/ml/public/cases/anomaly_charts_attachments.tsx

x-pack/plugins/ml/public/embeddables/anomaly_charts/initialize_anomaly_charts_controls.ts

x-pack/plugins/ml/public/embeddables/anomaly_charts/use_anomaly_charts_data.ts

…bed-migrate

…mponent

alvarezmelissa87

Latest changes LGTM ⚡

ThomThomson

Changes LGTM! Thank you for using the fetch$ interface. Left a few questions / nits.

ThomThomson · 2024-05-24T19:16:40Z

x-pack/plugins/ml/public/cases/anomaly_charts_attachments.tsx

+  const timeRange$ = useMemo(
+    () => new BehaviorSubject(initialState.timeRange),
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [initialState.timeRange?.from, initialState.timeRange?.to]


here we're creating a new behaviour subject every time the initialState timeRange changes? Instead, why not just .next the existing subject when those change?

We actually don't need this anymore so I've removed it here 34c2e07

ThomThomson · 2024-05-24T19:33:18Z

x-pack/plugins/ml/public/cases/anomaly_charts_attachments.tsx

+      return combined;
+    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    []


Because we've disabled exhaustive deps here, this component will not respond at all to changes in filters or query. Is this intentional?

If not, I'd suggest using a useEffect that updates these values with .next whenever the reactive variables change.

For the purpose for this attachment to case UI, the chart is static, not changing, and is rendered based on the initial state only. The query or filters don't and should not be be updated, so initializing these behaviors upon mounting is ok.

qn895 · 2024-05-26T23:11:52Z

@elasticmachine merge upstream

kibana-ci · 2024-05-27T00:39:12Z

💚 Build Succeeded

Buildkite Build
Commit: e4a7e78

Metrics [docs]

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.2MB	4.1MB	-101.9KB

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.1KB	80.1KB	-28.0B

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	106	105	-1

ESLint disabled line counts

id	before	after	diff
`ml`	549	556	+7

References to deprecated APIs

id	before	after	diff
`ml`	37	35	-2

Total ESLint disabled count

id	before	after	diff
`ml`	552	559	+7

History

💔 Build #212212 failed 3fab5fd
💔 Build #212208 failed 34c2e07
💔 Build #211864 failed 46d552e
💔 Build #211557 failed 403e9db
💔 Build #211540 failed 70d8bb0
💔 Build #211341 failed 83d95ff

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

cc @qn895

qn895 added 6 commits May 14, 2024 13:13

Initialize

25da1ce

Working chart

f927c6c

Filter by time range

1442b88

Remove old files

6656d55

Delete reference to createEditAnomalyChartsPanelAction

0773004

Add filters and open in anomlay explorer

5590512

qn895 added :ml Feature:Embeddables Relating to the Embeddable system v8.15.0 labels May 14, 2024

qn895 self-assigned this May 14, 2024

qn895 added 2 commits May 14, 2024 19:00

Merge remote-tracking branch 'upstream/main' into ml-anomaly-chart-em…

35f159d

…bed-migrate # Conflicts: # x-pack/plugins/ml/public/ui_actions/edit_single_metric_viewer_panel_action.tsx # x-pack/plugins/ml/public/ui_actions/index.ts

Migrate case attachment to new wrapped component instead of embeddable

eefef80

qn895 requested review from darnautov and alvarezmelissa87 May 16, 2024 15:27

ThomThomson added the project:embeddableRebuild label May 16, 2024

qn895 added 2 commits May 16, 2024 18:00

Clean up type

b925153

Update chart cursor to only update cursor and not the whole chart

72365f7

qn895 changed the title ~~[ML][Embeddables Rebuild] WIP - Migrate Anomaly Chart~~ [ML][Embeddables Rebuild] Migrate Anomaly Chart May 17, 2024

qn895 added the release_note:fix label May 17, 2024

qn895 added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels May 17, 2024

Update i18n

a4c3cad

Update tests

06b4123

Update flyout to use selection controls, update test, delete previous…

5aef100

… job flyout

qn895 force-pushed the ml-anomaly-chart-embed-migrate branch from 7d16ae2 to 5aef100 Compare May 17, 2024 19:54

qn895 added 2 commits May 21, 2024 14:14

Fix text overflowing or getting cut of weirdly, reporting

4ba962f

Update data-shared-item and data-rendering-count

83d95ff

qn895 commented May 21, 2024

View reviewed changes

ThomThomson reviewed May 21, 2024

View reviewed changes

qn895 added 5 commits May 21, 2024 19:58

Use theme for 4px, use fetch$

1ed16a4

Merge remote-tracking branch 'upstream/main' into ml-anomaly-chart-em…

2bc4623

…bed-migrate

Clean up embeddable type

70d8bb0

Consolidate reload$/refresh$ logic, fix spacing in flyout, remove con…

4a99e54

…sole

Merge remote-tracking branch 'upstream/main' into ml-anomaly-chart-em…

403e9db

…bed-migrate

peteharverson approved these changes May 22, 2024

View reviewed changes

darnautov reviewed May 23, 2024

View reviewed changes

qn895 added 6 commits May 23, 2024 15:01

Merge remote-tracking branch 'upstream/main' into ml-anomaly-chart-em…

0810da8

…bed-migrate

Rename component to clarify that it's a wrapper, not an embeddable co…

84136d7

…mponent

Rename to this.cursorStateSubscription for clarity

684972c

Refactor draw cursor

fb25cec

Update to use fetch$

81121a7

Fix types

46d552e

alvarezmelissa87 approved these changes May 24, 2024

View reviewed changes

ThomThomson approved these changes May 24, 2024

View reviewed changes

qn895 added 2 commits May 24, 2024 15:51

Remove redundant code, update type

34c2e07

Remove timeRange$

3fab5fd

Merge branch 'main' into ml-anomaly-chart-embed-migrate

e4a7e78

qn895 merged commit d901467 into elastic:main May 27, 2024
18 checks passed

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

darnautov mentioned this pull request May 27, 2024

[Embeddables Rebuild] Migrate Anomaly Chart #174966

Closed

rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024

[ML][Embeddables Rebuild] Migrate Anomaly Chart (elastic#183456)

166f38a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML][Embeddables Rebuild] Migrate Anomaly Chart #183456

[ML][Embeddables Rebuild] Migrate Anomaly Chart #183456

qn895 commented May 14, 2024 •

edited

qn895 commented May 17, 2024

qn895 commented May 17, 2024

qn895 commented May 17, 2024

qn895 commented May 21, 2024

qn895 May 21, 2024 •

edited

ThomThomson May 21, 2024

qn895 May 22, 2024

ThomThomson May 21, 2024

alvarezmelissa87 commented May 21, 2024

peteharverson left a comment

darnautov May 23, 2024

alvarezmelissa87 left a comment

ThomThomson left a comment

ThomThomson May 24, 2024

qn895 May 24, 2024

ThomThomson May 24, 2024

qn895 May 24, 2024

qn895 commented May 26, 2024

kibana-ci commented May 27, 2024

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

[ML][Embeddables Rebuild] Migrate Anomaly Chart #183456

[ML][Embeddables Rebuild] Migrate Anomaly Chart #183456

Conversation

qn895 commented May 14, 2024 • edited

Summary

Adding a new Anomaly chart panel in Dashboard:

Saving dashboard with settings like anomaly threshold persisted

Adding a new Anomaly chart to a case

Checklist

Risk Matrix

For maintainers

qn895 commented May 17, 2024

qn895 commented May 17, 2024

qn895 commented May 17, 2024

qn895 commented May 21, 2024

qn895 May 21, 2024 • edited

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

alvarezmelissa87 commented May 21, 2024

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarezmelissa87 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qn895 commented May 26, 2024

kibana-ci commented May 27, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

qn895 commented May 14, 2024 •

edited

qn895 May 21, 2024 •

edited