[ML] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788

walterra · 2024-02-13T07:52:04Z

Summary

Part of #176651 and #153476. Follow up to #175556.

Fixes the click on anomaly markers in the Single Metric Viewer embeddable. This required refactoring some dependencies to be properly passed in via React contexts instead of the legacy dependency cache.

smv-embeddable-click-0001.webm

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-02-13T16:07:10Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2024-02-13T16:38:26Z

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

-  if (dataViewsService === null) {
-    throw new Error('Data views are not initialized!');
-  }
+export async function getDataViewIdFromName(


Would it be worth putting these functions in a provider which takes the dataViewsService ?
I see there are a few places where this is called multiple times in the same file.

@walterra, @jgowdyelastic - this is already done via indexServiceFactory in ml/public/application/util/index_service.ts - this file will eventually replace this utils file.

Now I'm thinking it would have been clearer to add a comment to this soon-to-be-deprecated file saying it will be replaced by the one I mentioned above and that that one should be used.

Luckily all remaining usages of this function were in places where we could make the new service easily available so I was able to get rid of this stand-alone function in 70ab770. 🥳

alvarezmelissa87 · 2024-02-13T20:31:25Z

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

  const httpService = new HttpService(httpStart);
  const mlApiServices = mlApiServicesProvider(httpService);
+  const mlIndexUtils = indexServiceFactory(dataViews);
+  const mlFieldFormatService = fieldFormatServiceFactory(mlApiServices, mlIndexUtils);


To prevent bloat of the context, perhaps the mlFieldFormatService should be created in the component as needed and then passed/made available to it's children?

For example, we already have mlApiServices and dataViews in context, I believe.
fieldFormatServiceFactory requires mlApiServices and mlIndexUtils - which, in turn, only requires the dataViewsService.

From that, fieldFormatService could be instantiated once in the component in which it is needed instead of adding more to the context.

Not sure it's bloat? Prior to this PR the service was only instantiated once too, it's just that with the dependency cache under the hood it could be imported everywhere without being passed around via context. So this variant felt closer to the original then instantiating multiple times. Note the factory doesn't have any costly side effects, for example it doesn't call any requests, that's only happening once you call one of its methods.

I tend to agree that it would be better to keep mlIndexUtils and mlFieldFormatService out of this context as they are used so rarely throughout our plugin. It sets a precedence that all utils and services we have should go in here too.
Instantiating them when needed in each file would be better IMO.

As discussed offline, I'm leaving mlFieldFormatService here as a global service for now because of its internal state. mlIndexUtils will not be passed on to global services, instead it can be accessed within components via useMlIndexUtils().

I added the following comment to each place where the services are set up (the main ML app, the anomaly charts embeddable and the single metric viewer embeddable):

// Note on the following services: // - `mlIndexUtils` is just instantiated here to be passed on to `mlFieldFormatService`, // but it's not being made available as part of global services. Since it's just // some stateless utils `useMlIndexUtils()` should be used from within components. // - `mlFieldFormatService` is a stateful legacy service that relied on "dependency cache", // so because of its own state it needs to be made available as a global service. // In the long run we should again try to get rid of it here and make it available via // its own context or possibly without having a singleton like state at all, since the // way this manages its own state right now doesn't consider React component lifecycles.

Does this still require a code change? mlIndexUtils is still being added to the context.

Oh yes, overlooked that one, thanks! Fixed in bccd244.

alvarezmelissa87 · 2024-02-13T20:34:24Z

x-pack/plugins/ml/public/application/components/anomalies_table/links_menu.tsx

@@ -104,7 +104,7 @@ export const LinksMenuUI = (props: LinksMenuProps) => {

  const getAnomaliesMapsLink = async (anomaly: MlAnomaliesTableRecord) => {
    const index = job.datafeed_config.indices[0];
-    const dataViewId = await getDataViewIdFromName(index);
+    const dataViewId = await getDataViewIdFromName(data.dataViews, index);


As mentioned after James's comment below - the indexServiceFactory in ml/public/application/util/index_service.ts (which is meant to replace the utils file getDataViewIdFromName is pulled from here) should be used here to get that functionality. It only requires dataViewsService, which should be available in this component.

Updated in 70ab770.

alvarezmelissa87 · 2024-02-13T20:39:51Z

x-pack/plugins/ml/public/application/data_frame_analytics/common/use_results_view_config.ts

@@ -97,7 +97,7 @@ export const useResultsViewConfig = (jobId: string) => {

          try {
            const destIndex = getDestinationIndex(jobConfigUpdate);
-            const destDataViewId = (await getDataViewIdFromName(destIndex)) ?? destIndex;
+            const destDataViewId = (await getDataViewIdFromName(dataViews, destIndex)) ?? destIndex;


Same note here for either instantiating indexServiceFactory with the necessary services available via useMlKibana or passing in the service or function after instantiation wherever this hook is used.

This is the long term solution @jgowdyelastic and I chatted about for moving away from the dependency cache and also preventing making the context too bloated.

Updated in 70ab770.

alvarezmelissa87 · 2024-02-13T20:41:19Z

x-pack/plugins/ml/public/application/data_frame_analytics/pages/job_map/components/controls.tsx

@@ -138,7 +139,7 @@ export const Controls: FC<Props> = React.memo(
    const nodeType = selectedNode?.data('type');

    const onCreateJobClick = useCallback(async () => {
-      const dataViewId = await getDataViewIdFromName(nodeLabel);
+      const dataViewId = await getDataViewIdFromName(data.dataViews, nodeLabel);


Updated in 70ab770.

alvarezmelissa87 · 2024-02-13T20:45:10Z

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

  useEffect(() => {
+    const explorerService = explorerServiceFactory(mlFieldFormatService);


Same note here - mlFieldFormatService doesn't need to be pulled from context but can be instantiated in this component once (using the required services already in context) to be passed on.

alvarezmelissa87 · 2024-02-13T20:54:50Z

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

@@ -83,7 +85,10 @@ export class ExplorerChartDistribution extends React.Component {
      return;
    }

-    const fieldFormat = mlFieldFormatService.getFieldFormat(config.jobId, config.detectorIndex);
+    const fieldFormat = this.context.services.mlServices.mlFieldFormatService.getFieldFormat(


Same here for instantiating the fieldformatService on the fly.

alvarezmelissa87 · 2024-02-14T17:51:40Z

Gave this another test and getting blank charts now along with this toast:

Looks like the job service has a dependency cache dependency that could be causing issues. I think it tries to use the http service from the dependency cache.
Getting these errors in console:

For this ^ looks like the path might be wrong - I think it should be this.context.services.mlServices.mlApiServices.results

walterra · 2024-02-15T09:47:58Z

@alvarezmelissa87 Thanks for the testing and spotting the errors!

The first toast is not related to this PR, you need to update your ES instance because the Kibana data views service makes use of new functionality.
I was able to fix mlJobService to make it work in embeddables in 3ee830d.
Well spotted on the wrong context access for mlApiServices.results, that one's fixed too.

…-fix'

elasticmachine · 2024-02-15T10:25:20Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

…kibana into 176651-ml-smv-embeddable-click

kibana-ci · 2024-03-04T09:44:55Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: cef71e5

Failed CI Steps

Jest Tests #1

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`	3.7MB	3.7MB	+12.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`	79.0KB	78.4KB	-691.0B

Unknown metric groups

async chunk count

id	before	after	diff
`ml`	44	45	+1

ESLint disabled line counts

id	before	after	diff
`ml`	560	563	+3

Total ESLint disabled count

id	before	after	diff
`ml`	563	566	+3

History

💛 Build #194071 was flaky d4e6b66
💛 Build #193795 was flaky bcda568
💔 Build #193774 failed 9333edc
💚 Build #193662 succeeded bccd244
💛 Build #193560 was flaky f57c08d
💔 Build #193530 failed 70ab770

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

cc @walterra

peteharverson

Tested and LGTM

jgowdyelastic

LGTM

Fixing bug introduced in #176788 The `jobService` hasn't been initialised when being used to load the groups.

fix anomaly marker click in SMV embeddable

c07d8d7

walterra added the release_note:skip Skip the PR/issue when compiling release notes label Feb 13, 2024

walterra self-assigned this Feb 13, 2024

walterra added 8 commits February 13, 2024 10:03

remove console.logs

85dc3f2

dependencies

c84fedc

simplify anomaly explorer context

9ff3807

add comment

4abe2a9

fix dependencies

0ce7c8a

fix jest tests

5e479f4

fix embeddable anomaly chart

1512c2f

fix jest test

2fcc8b6

walterra mentioned this pull request Feb 13, 2024

[ML] Remove dependency cache #153476

Closed

59 tasks

remove unused dependency cache items

02b1dfc

walterra mentioned this pull request Feb 13, 2024

[ML] Anomaly Detection: Single Metric Viewer embeddable - 8.14 enhancements #176651

Closed

13 tasks

walterra marked this pull request as ready for review February 13, 2024 16:06

walterra requested a review from a team as a code owner February 13, 2024 16:06

walterra added bug Fixes for quality problems that affect the customer experience :ml Feature:Anomaly Detection ML anomaly detection v8.13.0 labels Feb 13, 2024

walterra requested review from alvarezmelissa87 and jgowdyelastic February 13, 2024 16:07

jgowdyelastic reviewed Feb 13, 2024

View reviewed changes

alvarezmelissa87 reviewed Feb 13, 2024

View reviewed changes

walterra added 4 commits February 14, 2024 11:55

add comments about services

ad49b54

move loadDataViewListItems to mlIndexUtils

42e89c1

destructure mlIndexUtils

f57c08d

don't return mlIndexUtils

bccd244

walterra added 2 commits February 15, 2024 08:16

Merge branch 'main' into 176651-ml-smv-embeddable-click

1a4f9b6

fix mlJobService

3ee830d

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9333edc

…-fix'

walterra requested a review from a team as a code owner February 15, 2024 10:25

botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Feb 15, 2024

walterra added v8.14.0 and removed v8.13.0 labels Feb 15, 2024

walterra added 2 commits February 15, 2024 12:48

Merge branch 'main' into 176651-ml-smv-embeddable-click

3f40ece

Merge branch '176651-ml-smv-embeddable-click' of github.com:walterra/…

bcda568

…kibana into 176651-ml-smv-embeddable-click

walterra removed the request for review from a team February 16, 2024 11:14

walterra added 2 commits February 19, 2024 08:31

Merge branch 'main' into 176651-ml-smv-embeddable-click

d4e6b66

Merge branch 'main' into 176651-ml-smv-embeddable-click

cef71e5

peteharverson approved these changes Mar 5, 2024

View reviewed changes

jgowdyelastic approved these changes Mar 5, 2024

View reviewed changes

walterra merged commit dda8c8f into elastic:main Mar 5, 2024
18 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Mar 5, 2024

walterra deleted the 176651-ml-smv-embeddable-click branch March 5, 2024 12:54

walterra removed the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Mar 5, 2024

walterra mentioned this pull request Mar 20, 2024

[ML][Meta] Technical debt and maintenance work for 8.14.0 #178782

Closed

14 tasks

jgowdyelastic mentioned this pull request Apr 15, 2024

[ML] Fixing groups dropdown in AD job wizards #180785

Merged

jgowdyelastic added a commit that referenced this pull request Apr 15, 2024

[ML] Fixing groups dropdown in AD job wizards (#180785)

e7e5105

Fixing bug introduced in #176788 The `jobService` hasn't been initialised when being used to load the groups.

walterra mentioned this pull request Apr 25, 2024

[ML] Popover menu for anomaly markers #181664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788

[ML] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788

walterra commented Feb 13, 2024 •

edited

Loading

elasticmachine commented Feb 13, 2024

jgowdyelastic Feb 13, 2024

alvarezmelissa87 Feb 13, 2024 •

edited

Loading

walterra Feb 14, 2024

alvarezmelissa87 Feb 13, 2024

walterra Feb 14, 2024

jgowdyelastic Feb 14, 2024

walterra Feb 14, 2024

jgowdyelastic Feb 14, 2024

walterra Feb 14, 2024

alvarezmelissa87 Feb 13, 2024

walterra Feb 14, 2024

alvarezmelissa87 Feb 13, 2024 •

edited

Loading

walterra Feb 14, 2024

alvarezmelissa87 Feb 13, 2024

walterra Feb 14, 2024

alvarezmelissa87 Feb 13, 2024

alvarezmelissa87 Feb 13, 2024

alvarezmelissa87 commented Feb 14, 2024

walterra commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

kibana-ci commented Mar 4, 2024

async chunk count

ESLint disabled line counts

Total ESLint disabled count

peteharverson left a comment

jgowdyelastic left a comment

		useEffect(() => {
		const explorerService = explorerServiceFactory(mlFieldFormatService);

[ML] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788

[ML] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788

Conversation

walterra commented Feb 13, 2024 • edited Loading

Summary

Checklist

elasticmachine commented Feb 13, 2024

Choose a reason for hiding this comment

alvarezmelissa87 Feb 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

alvarezmelissa87 Feb 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

alvarezmelissa87 commented Feb 14, 2024

walterra commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

kibana-ci commented Mar 4, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

walterra commented Feb 13, 2024 •

edited

Loading

alvarezmelissa87 Feb 13, 2024 •

edited

Loading

alvarezmelissa87 Feb 13, 2024 •

edited

Loading