[ML] Remove legacy scss overwrites Single Metric Viewer by rbrtj · Pull Request #195259 · elastic/kibana

rbrtj · 2024-10-07T13:36:09Z

Summary

Removes SCSS files for the Single Metric Viewer and adds BEM classes for annotations.
Affects the Single Metric Viewer in ML and the embeddable.
Part of #140695

kibana-ci · 2024-10-07T15:00:46Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #21 / machine learning - anomaly detection annotations creating creates annotation
[job] [logs] FTR Configs #21 / machine learning - anomaly detection annotations creating creates annotation
[job] [logs] Jest Tests #11 / Policy form Detect Prevent Protection level component should allow detect mode to be selected

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	2041	2038	-3

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	5.0MB	⚠️ +371.3KB

Page load bundle

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

id	before	after	diff
`ml`	74.4KB	74.4KB	+1.0B

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	109	108	-1

History

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

cc @rbrtj

elasticmachine · 2024-10-08T21:53:58Z

Pinging @elastic/ml-ui (:ml)

…-viewer

walterra · 2024-10-10T09:01:51Z

+    // Annotations styles
+
+    '.ml-annotation': {
+      '&__brush': {


This &__... syntax here and down below unfortunately isn't working after the migration. SCSS will use & to produce class names like .ml-annotation__brush, that's no longer happening so the correct styles are not getting applied to annotations.

This is how it looks on main:

This PR:

I believe the syntax is correct and it works!
What happened is (I'm guessing) that I deleted the @import 'timeseriesexplorer/index'; in
x-pack/plugins/ml/public/application/_index.scss
So the styles for annotations in the Anomaly Explorer were removed.
In 0d5af5f I split the styles into two separate functions and applied them where needed. It seems to be working correctly now. I've also updated the embeddables

Oh nice, great to see that's supported, so I was just misinterpreting why it was broken. I checked the latest state and it's working for me now too 👍

walterra · 2024-10-10T09:44:10Z

  const casesPermissions = cases?.helpers.canUseCases();
  const helpLink = docLinks.links.ml.anomalyDetection;
+
+  const timeseriesExplorerStyles = getTimeseriesExplorerStyles();


Could this be done once outside the component instead of during every render?

Done in 0d5af5f

walterra · 2024-10-10T09:44:35Z

    }
  };

+  const timeseriesExplorerStyles = getTimeseriesExplorerStyles();


Could this be done once outside the component instead of during every render?

Done in 0d5af5f

walterra

It's unfortunate that this change causes such an increase in async bundle sizes. Do you want to investigate a bit? You can do so by running this command:

node scripts/build_kibana_platform_plugins.js --dist --profile --focus=ml --no-cache

This will build optimized/minified bundles in x-pack/plugins/ml/target/public. In the directory you'll find some HTML files created by utilities to investigate the bundle sizes. You could compare main and this PR and investigate what's going on.

elasticmachine · 2024-10-10T13:36:22Z

💚 Build Succeeded

Buildkite Build
Commit: 0d5af5f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	2033	2030	-3

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.5MB	4.8MB	⚠️ +332.1KB

Page load bundle

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

id	before	after	diff
`ml`	75.0KB	75.0KB	+1.0B

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	109	107	-2

History

💚 Build #240766 succeeded 6884753
💔 Build #240636 failed 37fe887
💛 Build #240471 was flaky 1133603
💛 Build #240159 was flaky d5d917a

cc @rbrtj

…-viewer

rbrtj · 2024-10-11T08:29:11Z

It's unfortunate that this change causes such an increase in async bundle sizes. Do you want to investigate a bit? You can do so by running this command:
node scripts/build_kibana_platform_plugins.js --dist --profile --focus=ml --no-cache
This will build optimized/minified bundles in x-pack/plugins/ml/target/public. In the directory you'll find some HTML files created by utilities to investigate the bundle sizes. You could compare main and this PR and investigate what's going on.

It seems that the bundle size is good now. The problem was caused by importing styles into embeddable via index.ts instead of using a direct import, which caused some time-series-explorer components to appear twice in the bundle.

walterra

Latest changes including bundle size LGTM, great you were able to fix it!

elasticmachine · 2024-10-11T11:22:12Z

💚 Build Succeeded

Buildkite Build
Commit: e0cc1cc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	2035	2032	-3

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.5MB	4.5MB	-24.6KB

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	109	110	+1

History

💔 Build #241214 failed 2f1aa25
💔 Build #241175 failed 42b6ff4
💚 Build #241104 succeeded 0d5af5f
💚 Build #240766 succeeded 6884753
💔 Build #240636 failed 37fe887

cc @rbrtj

…-viewer

darnautov · 2024-10-14T08:44:26Z

+        },
+
+        '.tick line': {
+          stroke: euiThemeVars.euiColorLightShade,


does it look ok with the dark theme as well?

ah yes, I see it automatically adjust

darnautov

LGTM

elasticmachine · 2024-10-14T10:00:55Z

💚 Build Succeeded

Buildkite Build
Commit: d804cef

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	2035	2032	-3

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.5MB	4.5MB	-24.6KB

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	109	110	+1

History

💚 Build #241449 succeeded e0cc1cc
💔 Build #241214 failed 2f1aa25
💔 Build #241175 failed 42b6ff4
💚 Build #241104 succeeded 0d5af5f
💚 Build #240766 succeeded 6884753
💔 Build #240636 failed 37fe887

cc @rbrtj

kibanamachine · 2024-10-14T10:14:55Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11325480383

## Summary Removes SCSS files for the Single Metric Viewer and adds BEM classes for `annotations`. Affects the Single Metric Viewer in ML and the embeddable. Part of [elastic#140695](elastic#140695) (cherry picked from commit 87c91f4)

kibanamachine · 2024-10-14T10:19:32Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

… (#196085) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Remove legacy scss overwrites Single Metric Viewer (#195259)](#195259)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Robert Jaszczurek <92210485+rbrtj@users.noreply.github.com>

rbrtj added 3 commits October 7, 2024 14:30

use emotion instead of scss

314b277

bem for SMV annotations

3376bd3

update variable names

4e2c6c6

peteharverson mentioned this pull request Oct 7, 2024

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

Closed

rbrtj self-assigned this Oct 7, 2024

update test

d5d917a

rbrtj changed the title ~~Replace legacy scss overwrites single metric viewer~~ [ML] Remove legacy scss overwrites Single Metric Viewer Oct 8, 2024

add comment

1133603

rbrtj marked this pull request as ready for review October 8, 2024 21:52

rbrtj requested review from a team as code owners October 8, 2024 21:52

rbrtj requested review from darnautov and walterra October 8, 2024 21:53

rbrtj added :ml release_note:skip Skip the PR/issue when compiling release notes Team:ML Team label for ML (also use :ml) t// v8.16.0 backport:version Backport to applied version labels labels Oct 8, 2024

rbrtj added 2 commits October 9, 2024 13:09

Merge branch 'main' into replace-legacy-scss-overwrites-single-metric…

37fe887

…-viewer

Merge branch 'main' into replace-legacy-scss-overwrites-single-metric…

6884753

…-viewer

ryankeairns approved these changes Oct 9, 2024

View reviewed changes

walterra reviewed Oct 10, 2024

View reviewed changes

apply styles for anomaly explorer annotations & embeddables

0d5af5f

increased bundle size fix

42b6ff4

rbrtj and others added 2 commits October 10, 2024 16:38

import update

2f1aa25

Merge branch 'main' into replace-legacy-scss-overwrites-single-metric…

e0cc1cc

…-viewer

walterra approved these changes Oct 11, 2024

View reviewed changes

peteharverson mentioned this pull request Sep 19, 2024

[ML] Replace legacy SCSS overwrites with css-in-js #140695

Closed

71 tasks

Merge branch 'main' into replace-legacy-scss-overwrites-single-metric…

d804cef

…-viewer

darnautov reviewed Oct 14, 2024

View reviewed changes

darnautov approved these changes Oct 14, 2024

View reviewed changes

rbrtj merged commit 87c91f4 into elastic:main Oct 14, 2024

kibanamachine added the v9.0.0 label Oct 14, 2024

kibanamachine mentioned this pull request Oct 14, 2024

[8.x] [ML] Remove legacy scss overwrites Single Metric Viewer (#195259) #196085

Merged

Conversation

rbrtj commented Oct 7, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

kibana-ci commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

Uh oh!

elasticmachine commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 10, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

Uh oh!

rbrtj commented Oct 11, 2024

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 11, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

async chunk count

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 14, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

async chunk count

History

Uh oh!

kibanamachine commented Oct 14, 2024

Uh oh!

kibanamachine commented Oct 14, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

rbrtj commented Oct 7, 2024 •

edited by kibanamachine

Loading

kibana-ci commented Oct 7, 2024 •

edited

Loading