[ML] Anomaly Detection: Adds single metric viewer embeddable for dashboards by alvarezmelissa87 · Pull Request #175857 · elastic/kibana

alvarezmelissa87 · 2024-01-30T02:45:39Z

Summary

Related issue to add ability to insert "Single Metric Viewer" into a dashboard

This PR adds the single metric viewer as an embeddable that can be added to dashboards.

NOTE FOR TESTING:

This PR relies on the SMV fix for 'metric' jobs #176354
If that fix has not been merged, you will need to find getAnomalyRecordsSchema definition and add functionDescription: schema.maybe(schema.nullable(schema.string())), to it for local testing.

Screenshots of feature

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

x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_initializer.tsx

...ins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js

x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable.tsx

...imeseriesexplorer/timeseriesexplorer_embeddable_chart/timeseriesexplorer_embeddable_chart.js

peteharverson · 2024-02-02T13:04:41Z

.../plugins/ml/public/application/timeseriesexplorer/timeseriesexplorer_utils/get_focus_data.ts

+  anomalyRecords: MlAnomalyRecordDoc[];
  scheduledEvents: any;
-  showForecastCheckbox?: any;
+  showForecastCheckbox?: boolean;


Do you have forecast data showing up? I can't find a way of displaying data from a forecast in a chart. It might be best left for future work - it isn't needed for this first version, and you'd need to add a control into the initial modal for displaying which forecast to display.

Forecasting will be added in a follow up - though I left some of the code that will be needed for it since we will need it anyway.

x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable.tsx

qn895 · 2024-02-05T16:51:47Z

x-pack/plugins/ml/public/application/services/forecast_service_provider.ts

+        mlApiServices.results
+          .anomalySearch(
+            {
+              // @ts-ignore


We can probably remove these ts-ignore for the size param, or update it to ts-expect-error if we are expecting the type to be fixed in the future.

Added ts-expect error in c5181f1

qn895 · 2024-02-05T16:53:30Z

x-pack/plugins/ml/public/application/timeseriesexplorer/get_controls_for_detector.ts

 import { mlJobService } from '../services/job_service';
 import { Entity } from './components/entity_control/entity_control';
-import { JobId } from '../../../common/types/anomaly_detection_jobs';
+import { JobId, CombinedJob } from '../../../common/types/anomaly_detection_jobs';


nit: import type {}

Updated in c5181f1

jgowdyelastic · 2024-02-05T17:34:46Z

...lugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable_factory.ts

+  }
+
+  private async getServices(): Promise<SingleMetricViewerEmbeddableServices> {
+    const [coreStart, pluginsStart] = await this.getStartServices();


All of these awaited imports can be put inside a Promise.all

Good catch - updated in c5181f1

jgowdyelastic · 2024-02-05T19:38:07Z

...ins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js


    if (typeof selectedJob !== 'undefined') {
-      this.fieldFormat = mlFieldFormatService.getFieldFormat(selectedJob.job_id, detectorIndex);
+      this.fieldFormat = this.context?.services?.mlServices?.mlFieldFormatService


As we have the context here it looks like we can just instantiate mlFieldFormatService inside this class for both the embeddable and original use.

This one might be a bit trickier - I'll add it to the follow up tasks in the meta issue 👍

ok cool.
I didn't realise that mlFieldFormatService has state and stores the formats per job across components.
I think that should be changed in the future.

jgowdyelastic · 2024-02-05T19:40:49Z

x-pack/plugins/ml/public/application/app.tsx


-export type MlGlobalServices = ReturnType<typeof getMlGlobalServices>;
+export type MlGlobalServices = ReturnType<typeof getMlGlobalServices> & {
+  mlUtilsService?: {


It looks like we can probably get rid of this mlUtilsService and instantiate the providers for mlTimeBuckets and mlTimeSeriesExplorer inside the react classes.

Done in c5181f1

jgowdyelastic · 2024-02-05T19:42:19Z

...application/timeseriesexplorer/components/timeseries_chart/timeseries_chart_with_tooltip.tsx

  const {
    services: {
-      mlServices: { mlApiServices },
+      mlServices: { mlApiServices, mlUtilsService },


rather than using this mlUtilsService here, we can call timeBucketsProvider and pass it uiSettings

As per our chat, removed the need to have mlUtilsService in context at all - I was able to call the relevant providers and pass them the needed services already kept in context.
Changes in c5181f1

qn895 · 2024-02-05T21:35:31Z

x-pack/plugins/ml/public/application/services/forecast_service_provider.ts

+              // @ts-ignore
+              size: 1,
+              body: {
+                query: {


Looking into this more, we can probably get ride of the typescript error entirely if we put size param in the body {}:

Thanks for taking a look - I gave that a try but that seems to make the size property ignored by the request. Up for digging into it but seems outside the scope of this PR as these requests have just been copied over and have been working as is.

jgowdyelastic · 2024-02-06T12:01:48Z

...lugins/ml/public/embeddables/single_metric_viewer/use_single_metric_viewer_input_resolver.ts

+
+export function useSingleMetricViwerInputResolver(
+  embeddableInput: Observable<SingleMetricViewerEmbeddableInput>,
+  refresh: Observable<any>,


Looks like this could be Observable<void>

Updated in f682daa

jgowdyelastic · 2024-02-06T12:02:05Z

...ins/ml/public/embeddables/single_metric_viewer/embeddable_single_metric_viewer_container.tsx

+  embeddableContext: InstanceType<ISingleMetricViewerEmbeddable>;
+  embeddableInput: Observable<SingleMetricViewerEmbeddableInput>;
+  services: SingleMetricViewerEmbeddableServices;
+  refresh: Observable<any>;


Looks like this could be Observable<void>

Updated in f682daa

jgowdyelastic · 2024-02-06T15:00:00Z

...ins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js


    if (typeof selectedJob !== 'undefined') {
-      this.fieldFormat = mlFieldFormatService.getFieldFormat(selectedJob.job_id, detectorIndex);
+      this.fieldFormat = this.context?.services?.mlServices?.mlFieldFormatService


ok cool.
I didn't realise that mlFieldFormatService has state and stores the formats per job across components.
I think that should be changed in the future.

jgowdyelastic · 2024-02-06T15:06:43Z

We need a bit of padding on the right of the chart to allow the right brush handle to be visible.

It can still be grabbed, but you only have 1px to do it.

peteharverson

Gave it another cycle of testing and overall looking good. Only issue for me is sorting out the issue with metric detectors.

walterra

Looks good overall, just added some comments about naming! Good job digging through all the legacy code and get this to work!

It would be great if you could add comments in the code that indicate where code has been duplicated from and a corresponding issue that has references these comments. We did something similar for AIOps and Data Drift (see #174377).

...imeseriesexplorer/timeseriesexplorer_embeddable_chart/timeseriesexplorer_embeddable_chart.js

x-pack/plugins/ml/public/application/util/time_buckets_util.ts

x-pack/plugins/ml/public/application/util/timeseriesexplorer_utils.ts

alvarezmelissa87 · 2024-02-07T01:02:52Z

We need a bit of padding on the right of the chart to allow the right brush handle to be visible. It can still be grabbed, but you only have 1px to do it.

Updated in f682daa

alvarezmelissa87 · 2024-02-07T04:18:32Z

This has been updated with all comments and is ready for a final look when you get a chance cc @peteharverson, @jgowdyelastic, @walterra

I did plenty of testing but as this is a large change I would appreciate some help on testing from a couple of other people if possible. 🙏

walterra · 2024-02-07T14:42:10Z

Once you merge main, please check if the click action on anomaly markers in the single metric viewer charts works in the embedded version or if that's not added then because we duplicated the code (added in #175556). We need to decide then if this is something that should be fixed in this PR or a follow up.

x-pack/plugins/ml/public/application/util/timeseriesexplorer_utils.ts

x-pack/plugins/ml/public/application/services/field_format_service_provider.ts

x-pack/plugins/ml/public/application/util/index_utils_provider.ts

...pplication/timeseriesexplorer/timeseriesexplorer_utils/timeseries_search_service_provider.ts

alvarezmelissa87 · 2024-02-07T17:37:00Z

@elasticmachine merge upstream

peteharverson

Tested latest changes and LGTM.

x-pack/plugins/ml/public/application/services/forecast_service_provider.ts

walterra · 2024-02-08T10:02:02Z

...public/application/timeseriesexplorer/timeseriesexplorer_utils/time_series_search_service.ts

+// Note: This is a duplicated of `timeseries_search_service.ts` updated to move away from dependency cache.
+// The original file will be removed once all references are replaced with references to this file.


Suggest to replace comment with the following so we can search for the pattern // TODO Consolidate:

// TODO Consolidate with legacy code in // `x-pack/plugins/ml/public/application/timeseriesexplorer/timeseries_search_service.ts`

x-pack/plugins/ml/public/application/util/index_service.ts

x-pack/plugins/ml/public/application/util/time_buckets_service.ts

walterra · 2024-02-08T10:06:26Z

x-pack/plugins/ml/public/application/util/time_series_explorer_service.ts

+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+


Let's add a comment here like:

// TODO Consolidate with legacy code in // `ml/public/application/timeseriesexplorer/timeseriesexplorer_utils/timeseriesexplorer_utils.js`.

All comments for consolidation added in 1bd6b8b

walterra · 2024-02-08T10:18:41Z

The PR #175556 that adds the action menu for anomalies to Single Metric Viewer is now merged, but it doesn't work in the embedded version. Should be fixed in this PR or add as a follow up item in #173555.

jgowdyelastic

I agree with @walterra 's suggestion about tracking duplicate code using comments in the source, as they'll be easier to find later on.

Other than that, LGTM

alvarezmelissa87 · 2024-02-08T16:09:56Z

@elasticmachine merge upstream

walterra

Latest changes LGTM! 🥳

kibana-ci · 2024-02-08T18:30:40Z

💚 Build Succeeded

Buildkite Build
Commit: 1bd6b8b

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	1896	1919	+23

Async chunks

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

id	before	after	diff
`ml`	3.6MB	3.7MB	+66.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`	78.4KB	78.5KB	+90.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/ml-anomaly-utils`	206	207	+1

async chunk count

id	before	after	diff
`ml`	36	44	+8

ESLint disabled line counts

id	before	after	diff
`ml`	553	560	+7

References to deprecated APIs

id	before	after	diff
`ml`	24	27	+3

Total ESLint disabled count

id	before	after	diff
`ml`	556	563	+7

History

💚 Build #192033 succeeded 8dedbde
💔 Build #191788 failed 4b514c3
💛 Build #191737 was flaky f682daa
💚 Build #191505 succeeded 9078703
💔 Build #191499 failed ba542f1

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

cc @alvarezmelissa87

…oards (elastic#175857) ## Summary Related issue to [add ability to insert "Single Metric Viewer" into a dashboard](elastic#173555) This PR adds the single metric viewer as an embeddable that can be added to dashboards. ### NOTE FOR TESTING: This PR relies on the SMV fix for 'metric' jobs elastic#176354 If that fix has not been merged, you will need to find `getAnomalyRecordsSchema` definition and add `functionDescription: schema.maybe(schema.nullable(schema.string())),` to it for local testing. ### Screenshots of feature <img width="698" alt="image" src="https://github.com/elastic/kibana/assets/6446462/425e701a-3c9d-4a82-bf2e-1db5b3689165"> <img width="1193" alt="image" src="https://github.com/elastic/kibana/assets/6446462/e941ec1c-14f6-4723-b80c-71124f617dc9"> <img width="1209" alt="image" src="https://github.com/elastic/kibana/assets/6446462/dddd1dde-844c-47ae-ba94-61de5301746f"> <img width="1214" alt="image" src="https://github.com/elastic/kibana/assets/6446462/39439b4f-d296-4f3d-bdc9-4922553af6fa"> ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alvarezmelissa87 added 2 commits January 29, 2024 19:41

wip: create single metric viewer action in dashboard

c144d04

wip: add smv as embeddable that can be viewed in dashboards

a272e21

alvarezmelissa87 added :ml Feature:Anomaly Detection ML anomaly detection release_note:feature Makes this part of the condensed release notes v8.13.0 labels Jan 30, 2024

alvarezmelissa87 self-assigned this Jan 30, 2024

alvarezmelissa87 added 2 commits January 30, 2024 11:50

fix multiple charts issue

4cae098

fix slider listener for multiple smv charts

978f019

alvarezmelissa87 requested a review from peteharverson January 31, 2024 16:14

alvarezmelissa87 added 2 commits January 31, 2024 16:12

remove unnecessary comments

8333406

add unique id to checkbox

efce040

alvarezmelissa87 mentioned this pull request Feb 1, 2024

[ML] Anomaly Detection: Add ability to insert "Single Metric Viewer" into a dashboard #173555

Closed

22 tasks

alvarezmelissa87 requested a review from jgowdyelastic February 1, 2024 23:23

peteharverson reviewed Feb 2, 2024

View reviewed changes

x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable.tsx Show resolved Hide resolved

fix entity selection controls

e2e9388

alvarezmelissa87 requested a review from qn895 February 3, 2024 02:11

alvarezmelissa87 added 3 commits February 2, 2024 19:25

ensure panel settings are applied

d931543

remove annototaion creation when embeddable

c5304be

remove annotations help text for embeddable

85d0042

qn895 reviewed Feb 5, 2024

View reviewed changes

jgowdyelastic reviewed Feb 5, 2024

View reviewed changes

ensure type is correct

a744275

jgowdyelastic reviewed Feb 5, 2024

View reviewed changes

qn895 reviewed Feb 5, 2024

View reviewed changes

jgowdyelastic reviewed Feb 6, 2024

View reviewed changes

peteharverson reviewed Feb 6, 2024

View reviewed changes

walterra reviewed Feb 6, 2024

View reviewed changes

update naming

f682daa

fix metric jobs in SMV in dashboard and ML

4b514c3

walterra reviewed Feb 7, 2024

View reviewed changes

update naming from provider to factory for consistency

659f1ca

Merge branch 'main' into ml-ad-single-metric-embeddable

8dedbde

peteharverson approved these changes Feb 7, 2024

View reviewed changes

walterra reviewed Feb 8, 2024

View reviewed changes

jgowdyelastic approved these changes Feb 8, 2024

View reviewed changes

kibanamachine and others added 2 commits February 8, 2024 11:10

Merge branch 'main' into ml-ad-single-metric-embeddable

39988a0

add comment to consolidate all duplicated services

1bd6b8b

walterra approved these changes Feb 8, 2024

View reviewed changes

alvarezmelissa87 merged commit ee34012 into elastic:main Feb 8, 2024

kibanamachine added the backport:skip This PR does not require backporting label Feb 8, 2024

alvarezmelissa87 deleted the ml-ad-single-metric-embeddable branch February 8, 2024 19:32

peteharverson changed the title ~~[ML] Anomaly Detection: Add single metric viewer embeddable for dashboards~~ [ML] Anomaly Detection: Adds single metric viewer embeddable for dashboards Feb 14, 2024

timductive mentioned this pull request Mar 6, 2024

[Embeddables Rebuild] Migrate Single Metric Viewer #178130

Closed

		// Note: This is a duplicated of `timeseries_search_service.ts` updated to move away from dependency cache.
		// The original file will be removed once all references are replaced with references to this file.

Conversation

alvarezmelissa87 commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

NOTE FOR TESTING:

Screenshots of feature

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

jgowdyelastic commented Feb 6, 2024

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alvarezmelissa87 commented Feb 7, 2024

Uh oh!

alvarezmelissa87 commented Feb 7, 2024

Uh oh!

walterra commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

alvarezmelissa87 commented Jan 30, 2024 •

edited

Loading