[ML] Add support for per-partition categorization jobs #74592

qn895 · 2020-08-06T20:11:48Z

Summary

Part of meta issue #73968 which adds the ability check for per-partition in categorization job creation wizard.

UI-related changes

Categorization job creation wizard
- Navigating to next screen and back should persist the settings
- Switching to a different type of detector after setting the per-partition field name should persist the partition_field_name in the detector config
- Turning off per_partition should also set stop_on_warn to false

Advanced job creation wizard (when a category field is selected)
If viewing the results of a per-partition categorization job in the Anomaly Explorer, and if stop_on_warn was enabled, add an indicator somewhere in the view if the categorization status goes to warn for a partition. This is needed to explain to the user that there may be fewer results than there could have been.

API-related

Added api/ml/anomaly_detectors/{jobId}/categorizer_stats for retrieving categorizer_stats documents
Added api/ml/anomaly_detectors/{jobId}/stopped_partitions to find out which partitions we stopped categorizing (searching for categorizer_stats documents in the job's results index)

1. First get the job config and check if analysis_config.per_partition_categorization.stop_on_warn is true
2. If so, search for categorizer_stats documents for the current job where the categorization_status is warn
3. Return all the partition_field_value values from the documents found
If analysis_config.per_partition_categorization.stop_on_warn is false then we won’t have stopped categorizing anything, so you can return an empty list

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

…rtitions

…n_name

jgowdyelastic · 2020-08-06T21:30:09Z

x-pack/plugins/ml/server/routes/anomaly_detectors.ts

@@ -696,4 +699,166 @@ export function jobRoutes({ router, mlLicense }: RouteInitialization) {
      }
    })
  );
+
+  /**
+   * @apiGroup AnomalyDetectors


these endpoints are better suited to job_service, as this file only contains wrappers for the anomaly_detectors es endpoints.
there is a categorisation section inside here: https://github.com/elastic/kibana/tree/master/x-pack/plugins/ml/server/models/job_service/new_job/categorization
but as these are used for results, they might need a different folder, or maybe should live in results_service

That's a good point. Now that I see it, I think it's probably more suitable for results_service. Will update it to be that group instead. Thanks Jame!

…r-partition # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

jgowdyelastic · 2020-08-10T08:39:08Z

x-pack/plugins/ml/server/models/results_service/results_service.ts

+        },
+      });
+    }
+    const results: SearchResponse<AnomalyCategorizerStatsDoc> = await callAsCurrentUser('search', {


all searches to ML_RESULTS_INDEX_PATTERN should be performed by the internal user.

…port multiple jobs

elasticmachine · 2020-08-10T22:41:45Z

Pinging @elastic/ml-ui (:ml)

pheyos

Thanks for adding API tests as part of this PR! 🎉
Left a few comments.

...fields_step/components/categorization_partition_field/categorization_per_partition_input.tsx

...ields_step/components/categorization_partition_field/categorization_per_partition_switch.tsx

...fields_step/components/categorization_partition_field/categorization_stop_on_warn_switch.tsx

x-pack/test/api_integration/apis/ml/results/get_categorizer_stats.ts

x-pack/test/api_integration/apis/ml/results/get_stopped_partitions.ts

…r-partition # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

qn895 · 2020-08-11T16:57:43Z

Updated PR with the latest changes and I think it's ready for a final look @jgowdyelastic, @alvarezmelissa87, @pheyos 🙏

darnautov · 2020-08-12T08:37:55Z

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

+            {stoppedPartitions && (
+              <EuiCallOut
+                size={'s'}
+                title={i18n.translate('xpack.ml.explorer.stoppedPartitionsExistCallout', {


It's recommended to use { FormattedMessage } from '@kbn/i18n/react'; inside of components instead of i18n.

Updated all the i18n.translate to using FormattedMessage here 3686b73

darnautov · 2020-08-12T08:40:19Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

@@ -57,6 +57,7 @@ export class JobCreator {
  private _stopAllRefreshPolls: {
    stop: boolean;
  } = { stop: false };
+  private _partitionField: string | null;


nit: use

Suggested change

private _partitionField: string | null;

private _partitionField: string | null = null;

and there is no need to defined it in the constructor.

After discussing with James, we decided to remove this _partitionField altogether here 0a813e1

darnautov · 2020-08-12T08:42:03Z

...pplication/jobs/new_job/pages/components/pick_fields_step/components/advanced_view/extra.tsx

@@ -21,6 +27,13 @@ export const ExtraSettings: FC = () => {
          <SummaryCountField />
        </EuiFlexItem>
      </EuiFlexGroup>
+      {showCategorizationPerPartitionField && (
+        <EuiFlexGroup gutterSize="xl">


do you need a flex group just for one item?

Removed here a94ecaf

darnautov · 2020-08-12T08:43:41Z

.../pick_fields_step/components/categorization_partition_field/categorization_per_partition.tsx

+  CategorizationJobCreator,
+  isCategorizationJobCreator,
+} from '../../../../../common/job_creator';
+import { newJobCapsService } from '../../../../../../../services/new_job_capabilities_service';


Please avoid using static imports for services. Put them in a context instead.

i agree this would be better in a context, however newJobCapsService is used throughout the wizards and so i'd suggest this happens in a refactoring PR later on.

I'll save this issue for a follow up PR like James mentioned

darnautov · 2020-08-12T08:48:32Z

...fields_step/components/categorization_partition_field/categorization_per_partition_input.tsx

+  const options: EuiComboBoxOptionOption[] = [
+    ...createFieldOptions(fields, jobCreator.additionalFields),
+  ];
+
+  const selection: EuiComboBoxOptionOption[] = [];
+  if (selectedField !== null) {
+    selection.push({ label: selectedField });
+  }
+
+  function onChange(selectedOptions: EuiComboBoxOptionOption[]) {
+    const option = selectedOptions[0];
+    if (typeof option !== 'undefined') {
+      changeHandler(option.label);
+    } else {
+      changeHandler(null);
+    }
+  }


It all gets invoked on each render. Please use hooks accordingly (useMemo and useCallback)

Updated here a94ecaf

darnautov · 2020-08-12T08:54:25Z

x-pack/plugins/ml/public/application/routing/routes/explorer.tsx

+      }
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.error(error);


do we want to fail silently here?

I think for this scenario it's okay to fail silently since it's only there to give a warning/info message if one of the partition has stopped. But if we think it's important for the user to know, I can add an error message.

darnautov · 2020-08-12T09:01:04Z

x-pack/plugins/ml/server/models/results_service/results_service.ts

+    });
+
+    if (!jobConfigResponse || jobConfigResponse.jobs.length < 1) {
+      throw Error(`Unable to find anomaly detector jobs ${jobIds.join(', ')}`);


shall we use throw Boom.notFound instead? to return an error with appropriate code

Updated here a94ecaf

darnautov · 2020-08-12T09:05:48Z

x-pack/plugins/ml/server/routes/results_service.ts

+   * @api {get} /api/ml/results/:jobId/categorizer_stats
+   * @apiName GetStoppedPartitions
+   * @apiDescription Returns list of partitions we stopped categorizing whens status changed to warn
+   * @apiSchema (params) jobIdSchema


should be

Suggested change

* @apiSchema (params) jobIdSchema

* @apiSchema (body) getStoppedPartitionsSchema

Updated here a94ecaf

darnautov · 2020-08-12T09:06:23Z

x-pack/plugins/ml/server/routes/results_service.ts

+  /**
+   * @apiGroup ResultsService
+   *
+   * @api {get} /api/ml/results/:jobId/categorizer_stats


should be

Suggested change

* @api {get} /api/ml/results/:jobId/categorizer_stats

* @api {post} /api/ml/results/stopped_partitions

Updated here a94ecaf

darnautov · 2020-08-12T09:07:33Z

x-pack/plugins/ml/server/routes/results_service.ts

+  /**
+   * @apiGroup ResultsService
+   *
+   * @api {get} /api/ml/results/:jobId/categorizer_stats


It requires some brief description for docs to be properly generated, please check other examples

Updated here a94ecaf

jgowdyelastic · 2020-08-12T09:37:32Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

+    this._job_config.analysis_config.per_partition_categorization!.enabled = enabled;
+  }
+
+  public get partitionStopOnWarn() {


should this name be perPartitionStopOnWarn ?

Updated here a94ecaf

jgowdyelastic · 2020-08-12T09:51:46Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

@@ -81,6 +82,7 @@ export class JobCreator {
    }

    this._datafeed_config.query = query;
+    this._partitionField = null;


keeping a separate copy of the partition field name will lead to this becoming out of sync with the real partition field name in the job config, if the user edits the job's JSON.
i think we should be using the partition field from the detector which uses the mlcategory field.

this will make the get categorizationPerPartitionField() function trickier as it'll have to find that field each time it is called.
Perhaps a workaround would be to store the index of the categorisation detector in this class for fast look up. in the categorisation wizard that will always be 0.

I don't see how we can deal with multiple categorisation detectors with differing partition fields in the UI. Even though it is possible to configure in the JSON.

As from our conversation, I have removed this._partitionField altogether. Since storing the index might lead to the same issue where the index being out of sync with the detectors (e.g. like deleting one of the detector), I have changed the getter function to find the first instance of detector where the keyword mlcategory exists. Since the partition_field_name property has to be the same value in every detector that uses the keyword mlcategory, I think this change is more suitable.

jgowdyelastic · 2020-08-12T10:04:30Z

.../pages/components/pick_fields_step/components/categorization_partition_field/description.tsx

+import { EuiDescribedFormGroup } from '@elastic/eui';
+
+interface Props {
+  isOptional: boolean;


isOptional isn't needed for this component

Updated in a94ecaf

jgowdyelastic · 2020-08-12T10:07:01Z

.../pick_fields_step/components/categorization_partition_field/categorization_per_partition.tsx

+  CategorizationJobCreator,
+  isCategorizationJobCreator,
+} from '../../../../../common/job_creator';
+import { newJobCapsService } from '../../../../../../../services/new_job_capabilities_service';


i agree this would be better in a context, however newJobCapsService is used throughout the wizards and so i'd suggest this happens in a refactoring PR later on.

jgowdyelastic · 2020-08-12T10:09:44Z

.../pick_fields_step/components/categorization_partition_field/categorization_per_partition.tsx

+      jobCreator.perPartitionCategorization &&
+      jobCreator.categorizationPerPartitionField === null
+    ) {
+      jobCreator.categorizationPerPartitionField = filteredCategories[0].id;


just to be safe, it would be worth adding a check for filteredCategories.length before access it.

Updated in a94ecaf

jgowdyelastic · 2020-08-12T10:18:22Z

x-pack/plugins/ml/public/application/services/ml_api_service/results.ts

+      jobIds,
+      fieldToBucket,
+    });
+    return httpService.http<any>({


looks like this returns a GetStoppedPartitionResult
That interface be moved to a common location and used server side and here.

Updated in a94ecaf

jgowdyelastic · 2020-08-12T10:29:14Z

x-pack/plugins/ml/server/routes/results_service.ts

@@ -71,6 +76,19 @@ function getPartitionFieldsValues(legacyClient: ILegacyScopedClusterClient, payl
  return rs.getPartitionFieldsValues(jobId, searchTerm, criteriaFields, earliestMs, latestMs);
 }

+function getCategorizerStats(context: RequestHandlerContext, params: any, query: any) {


this needs updating to use legacyClient

Updated in a94ecaf

- Refactor context -> legacyclient - Update naming & formatted strings - Update schema - Update some functions to useCallback or useMemo - Rename perPartitionStopOnWarn

jgowdyelastic · 2020-08-13T07:39:00Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

+    // looping through to find current partition_field name to prevent stale/syncing issue
+    // possible because partition_field_name has to have same value in every detector that uses the keyword mlcategory
+    const firstCategorizationDetector = this._detectors.find(
+      (d) => d.by_field_name === 'mlcategory'


we have a common constant for this string MLCATEGORY in field_types

jgowdyelastic · 2020-08-13T07:40:45Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

+    return null;
+  }
+
+  public set categorizationPerPartitionField(fieldName: string | null) {


this should only be changing detectors which are using mlcategory as the by_field_name

jgowdyelastic · 2020-08-13T07:55:23Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

+    );
+    if (
+      firstCategorizationDetector &&
+      'partition_field_name' in firstCategorizationDetector &&


I don't think this check is needed as the undefined below has it covered.

jgowdyelastic · 2020-08-13T08:04:15Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/job_creator.ts

+        delete detector.partition_field_name;
+      });
+    } else {
+      if (this.categorizationPerPartitionField !== fieldName) {


thinking about this a bit more, this check won't catch the situation where a second mlcategory detector has been added with a different partition field.
I think we're going to struggle to add nice UI behaviour in this situation without looping over the detectors whenever one is added to check for mlcategory.

Just tested this and validation fails in a bad way when we have two categorisation detectors with different partition fields.
I think we should allow this to be configured in the UI, and we should fix validation so it displays the error in the correct way.

After chatting to @droberts195 we agree that the advanced job wizard shouldn't have a global way to configure the partition field for all categorisation detectors.
Instead the user should do this themselves per detector.
This means the categorizationPerPartitionField can be moved out of the base JobCreator class and instead live in the derived CategorizationJobCreator class.
It also means we can put the partitionField variable back in as there will only ever be one detector in that job.

pheyos

Functional tests LGTM

…r-partition # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.searchinterceptor.md # src/plugins/data/server/index.ts # src/plugins/data/server/server.api.md # x-pack/plugins/reporting/server/routes/jobs.ts # x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx # x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx # x-pack/plugins/security_solution/public/resolver/view/panel.test.tsx # x-pack/test/functional/apps/reporting_management/report_listing.ts

kibanamachine · 2020-08-14T01:45:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5298fb3
Pipeline Steps (look for red circles / failed steps)
Interpreting CI Failures

Failed CI Steps

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
maps	692	-1	693
ml	1179	-176	1355
total		-177

async chunks size

id	value	diff	baseline
maps	3.3MB	+21.2KB	3.3MB
ml	7.9MB	-71.2KB	8.0MB
securitySolution	7.2MB	-1.0KB	7.2MB
visTypeVega	1.4MB	-278.0B	1.4MB
total		-51.3KB

page load bundle size

id	value	diff	baseline
data	1.4MB	-1.7KB	1.4MB
dataEnhanced	177.9KB	-111.0B	178.0KB
lens	843.9KB	-14.0KB	858.0KB
ml	523.2KB	-50.0KB	573.2KB
securitySolution	805.8KB	-72.0B	805.9KB
visTypeVega	661.1KB	-220.0B	661.3KB
total		-66.1KB

History

💚 Build #67763 succeeded 3686b73
💔 Build #67554 failed 1f3c5ec
💚 Build #67371 succeeded 98bb431
💔 Build #67138 failed abe77e7

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

qn895 · 2020-08-14T13:40:11Z

Seems like I have accidentally pulled Dima's PR in as well with my updates yesterday. Will close this PR and create a new one. My apologies for the convenience.

qn895 added 7 commits August 6, 2020 11:34

[ML] Add option to add per-partition categorization for cat jobs

5cb13f5

[ML] Add per partition endpoints for categorizer_stats and stopped_pa…

6a3e91f

…rtitions

[ML] Add callout on AE page if there's a stopped partition

283db47

[ML] Add schema to get stopped_partitions for multiple jobs

9b513cf

[ML] Add support for per-partition advanced categorization wizard

10746ca

[ML] Remove unused getStoppedPartitionsSchema

ae61c2a

[ML] Fix so if cat detector changes type it will accommodate partitio…

ad453e3

…n_name

qn895 added release_note:enhancement :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.10.0 labels Aug 6, 2020

qn895 self-assigned this Aug 6, 2020

qn895 added 2 commits August 6, 2020 15:28

[ML] Update query size for stopped_partition to 0 since it's unused

cbb857c

[ML] Fix random anomalyExplorerLabel

5aaf226

jgowdyelastic reviewed Aug 6, 2020

View reviewed changes

qn895 added 4 commits August 9, 2020 21:35

[ML] Move logic to results service from anomaly_detectors

1591562

[ML] Fix untranslated strings

a35ec15

[ML] Refactor core logic to Results model

abe77e7

jgowdyelastic reviewed Aug 10, 2020

View reviewed changes

qn895 added 3 commits August 10, 2020 09:28

[ML] Update callAsInternalUser and text descriptions

f0a3cf4

[ML] Update callAsInternalUser and text descriptions, refactor to sup…

5ddeaab

…port multiple jobs

[ML] Update callAsInternalUser and text descriptions, refactor to sup…

98bb431

…port multiple jobs

qn895 marked this pull request as ready for review August 10, 2020 22:41

qn895 requested a review from a team as a code owner August 10, 2020 22:41

mukeshelastic mentioned this pull request Aug 11, 2020

[Logs UI] Include the dataset information in categorization warning message #60392

Closed

pheyos reviewed Aug 11, 2020

View reviewed changes

[ML] Update namings and expect for partition field

9ca076b

qn895 added 2 commits August 11, 2020 11:39

[ML] Update tests to reflect API changes

bb698b6

qn895 requested review from alvarezmelissa87 and jgowdyelastic August 11, 2020 16:56

pheyos added the test-api-integration label Aug 12, 2020

darnautov reviewed Aug 12, 2020

View reviewed changes

jgowdyelastic reviewed Aug 12, 2020

View reviewed changes

darnautov and others added 8 commits August 12, 2020 15:02

[ML] init tabs

e796419

[ML] init inference API service in UI

02e17b3

[ML] server-side routes

6620328

[ML] basic table

646ef99

[ML] Update naming and refactor context

a94ecaf

- Refactor context -> legacyclient - Update naming & formatted strings - Update schema - Update some functions to useCallback or useMemo - Rename perPartitionStopOnWarn

[ML] Change i18n to FormattedMessage

621d3c5

[ML] Remove this._partitionField

0a813e1

[ML] Fix type errors

3686b73

jgowdyelastic reviewed Aug 13, 2020

View reviewed changes

pheyos approved these changes Aug 13, 2020

View reviewed changes

darnautov and others added 8 commits August 13, 2020 14:37

[ML] support deletion

c5a56ca

[ML] delete multiple models

3801a38

[ML] WIP expanded row

dd2f28c

[ML] fix types

96e6919

[ML] expanded row

976b46a

[ML] fix types

043f4f2

[ML] Change advanced wizard to not have the dropdown per partition field

b48cda6

qn895 closed this Aug 14, 2020

qn895 mentioned this pull request Aug 14, 2020

[ML] Add option for per-partition categorization to categorization job wizard #75061

Merged

14 tasks

	private _partitionField: string \| null;
	private _partitionField: string \| null = null;

	* @apiSchema (params) jobIdSchema
	* @apiSchema (body) getStoppedPartitionsSchema

	* @api {get} /api/ml/results/:jobId/categorizer_stats
	* @api {post} /api/ml/results/stopped_partitions

[ML] Add support for per-partition categorization jobs #74592

[ML] Add support for per-partition categorization jobs #74592

Conversation

qn895 commented Aug 6, 2020 • edited Loading

Summary

Checklist

jgowdyelastic Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 10, 2020

pheyos left a comment

Choose a reason for hiding this comment

qn895 commented Aug 11, 2020 • 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

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

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

jgowdyelastic Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

pheyos left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 14, 2020 • edited Loading

💔 Build Failed

Failed CI Steps

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

qn895 commented Aug 14, 2020

qn895 commented Aug 6, 2020 •

edited

Loading

jgowdyelastic Aug 6, 2020 •

edited

Loading

qn895 commented Aug 11, 2020 •

edited

Loading

jgowdyelastic Aug 13, 2020 •

edited

Loading

kibanamachine commented Aug 14, 2020 •

edited

Loading