[ML] Add option to Advanced Settings to set default time range filter for AD jobs #76347

qn895 · 2020-09-01T05:54:52Z

Summary

This PR adds Advanced Setting for anomaly detection default time filter time range.

When the setting is enabled, links to the Single Metric Viewer and the Anomaly Explorer from the Jobs Management list would be using the filter by default.

Current it supports quick options as well as "absolute" options:

{
  "from": "now-30d",
  "to": "now-15m"
}

Although more validations would be needed if there's any invalid terms for either "from" or "to".

Also updated:

Apply timerange -> Apply time range
The pixels for the job group controls to match the one for the job controls

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

qn895 · 2020-09-01T05:58:33Z

x-pack/plugins/ml/common/constants/settings.ts

@@ -5,3 +5,5 @@
 */

 export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
+export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeEnable';


Setting the name this way because seems like the settings are ordered by these strings alphabetically.

peteharverson · 2020-09-01T16:43:15Z

If I try and open the Jobs lists on the Stack Management page, I get this error saying the uiSettings haven't been initialized:

The links on that page should also use this new setting.

lcawl · 2020-09-01T18:19:39Z

x-pack/plugins/ml/common/constants/settings.ts

@@ -5,3 +5,5 @@
 */

 export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
+export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeEnable';
+export const ANOMALY_DETECTION_DEFAULT_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeSet';


Since this setting looks very similar to timepicker:timeDefaults, I wonder if we should give it a similar name?

Suggested change

export const ANOMALY_DETECTION_DEFAULT_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeSet';

export const ANOMALY_DETECTION_DEFAULT_TIME_RANGE = 'ml:anomalyDetectionTimeDefaults';

lcawl

I've added some text suggestions.

lcawl · 2020-09-01T18:28:53Z

x-pack/plugins/ml/server/lib/register_settings.ts

@@ -30,5 +34,41 @@ export function registerKibanaSettings(coreSetup: CoreSetup) {
        }),
      },
    },
+    [ANOMALY_DETECTION_ENABLE_TIME_RANGE]: {
+      name: i18n.translate('xpack.ml.advancedSettings.enableAnomalyDetectionDefaultTimeRangeName', {
+        defaultMessage: 'Enable default time range for anomaly detection jobs.',


If I'm understanding correctly and these two settings must be used together, I suggest synching the title more closely:

Suggested change

defaultMessage: 'Enable default time range for anomaly detection jobs.',

defaultMessage: 'Enable time filter defaults',

I changed it to 'Enable time filter defaults for anomaly detection jobs' because this setting is specific to that area

41af491

lcawl · 2020-09-01T18:35:24Z

x-pack/plugins/ml/server/lib/register_settings.ts

+      description: i18n.translate(
+        'xpack.ml.advancedSettings.anomalyDetectionDefaultTimeRangeDesc',
+        {
+          defaultMessage: 'Use a default time range to view anomaly detection jobs.',


If I'm understanding correctly that this applies only to specific interfaces, I suggest we mention them here:

Suggested change

defaultMessage: 'Use a default time range to view anomaly detection jobs.',

defaultMessage: 'Use the default time filter in the Single Metric Viewer and Anomaly Explorer.',

Updated here 41af491

lcawl · 2020-09-01T18:40:39Z

x-pack/plugins/ml/server/lib/register_settings.ts

+      description: i18n.translate(
+        'xpack.ml.advancedSettings.anomalyDetectionDefaultTimeRangeDesc',
+        {
+          defaultMessage: 'The default time range to view anomaly detection jobs.',


I suggest we use text closer to what's used for the timepicker:timeDefaults. For example:

Suggested change

defaultMessage: 'The default time range to view anomaly detection jobs.',

defaultMessage: 'The time filter selection to use when viewing anomaly detection job results.',

Updated here 41af491

lcawl · 2020-09-01T18:47:16Z

x-pack/plugins/ml/server/lib/register_settings.ts

+      description: i18n.translate(
+        'xpack.ml.advancedSettings.anomalyDetectionDefaultTimeRangeDesc',
+        {
+          defaultMessage: 'The default time range to view anomaly detection jobs.',


The default time range...

If you want a link to the list of accepted formats, perhaps it can be copied from the way data.advancedSettings.timepicker.quickRangesText uses {acceptedFormatsLink} in ui_settings.ts. I'm not convinced that link is necessary here, however.

lcawl · 2020-09-01T18:51:39Z

x-pack/plugins/ml/common/constants/settings.ts

@@ -5,3 +5,5 @@
 */

 export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
+export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeEnable';


If you accept my suggestion for renaming the other setting, this one should be updated to match:

Suggested change

export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeEnable';

export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionTimeDefaultsEnable';

Updated here 4155201 ... although now the order is changed where the toggle to set it comes after the json field. Not totally ideal but I'm not sure what's a good solution around making the names in alphabetical order.

…e + update field values

qn895 · 2020-09-01T21:26:31Z

If I try and open the Jobs lists on the Stack Management page, I get this error saying the uiSettings haven't been initialized:

Thanks for catching that Pete. I removed the cache dependency entirely for these link generating utils and instead pass in the useMlKibana contexts to prevent this problem in the future.

The links should now work as intended in the Job Management as well as the Overview Page.

I also made it so that if the user sets the date format incorrect (e.g. 'now-15mzzz'), the job will resets to the full range of the data, and show the following warn toast notification in AE/SMV pages:

elasticmachine · 2020-09-01T21:26:38Z

Pinging @elastic/ml-ui (:ml)

peteharverson · 2020-09-02T14:25:46Z

x-pack/plugins/ml/server/lib/register_settings.ts

+        'xpack.ml.advancedSettings.enableAnomalyDetectionDefaultTimeRangeDesc',
+        {
+          defaultMessage:
+            'Use the default time filter in the Single Metric Viewer and Anomaly Explorer.',


I think this needs an extra sentence to explain what happens if this is not enabled. Something like If not enabled, the results for the full time range of the job will be displayed.

Something like ... "If not enabled, the results....

LGTM, except I'd replace "will be" with "are".

Updated it to Use the default time filter in the Single Metric Viewer and Anomaly Explorer. If not enabled, the results for the full time range of the job are displayed. here 1118b3e

peteharverson · 2020-09-02T14:26:59Z

x-pack/plugins/ml/server/lib/register_settings.ts

@@ -30,5 +34,43 @@ export function registerKibanaSettings(coreSetup: CoreSetup) {
        }),
      },
    },
+    [ANOMALY_DETECTION_ENABLE_TIME_RANGE]: {
+      name: i18n.translate('xpack.ml.advancedSettings.enableAnomalyDetectionDefaultTimeRangeName', {
+        defaultMessage: 'Enable time filter defaults for anomaly detection jobs',


Or as this only applies to the results pages, should this be Enable time filter defaults for anomaly detection results?

Updated here 1118b3e

x-pack/plugins/ml/server/lib/register_settings.ts

peteharverson

Some more text suggestions!

peteharverson · 2020-09-02T14:37:02Z

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

+      toastNotifications.addWarning(
+        i18n.translate('xpack.ml.explorer.invalidTimeRangeInUrlCallout', {
+          defaultMessage:
+            'The time filter changed to the full range for this job due to invalid default time filter. Please check the advanced settings for {field}.',


Nit - suggest changing to The time filter was changed to the full range for this job due to an invalid default time filter. Please check the advanced settings for {field}.

Please check the advanced settings.

Another nit: The style guidelines recommend omitting "Please". I'd suggest "Check the advanced settings...".

Updated it to The time filter was changed to the full range due to an invalid default time filter. Check the advanced settings for {field}. here 1118b3e

peteharverson · 2020-09-02T14:40:45Z

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

+          defaultMessage:
+            'The time filter changed to the full range for this job due to invalid default time filter. Please check the advanced settings for {field}.',
+          values: {
+            field: ANOMALY_DETECTION_DEFAULT_TIME_RANGE,


The message either needs changing to remove for this job or else you need to pass in the number of jobs selected in the view and change to for these jobs if more than one is selected.

I removed the for this job part to make it more concise and I think it reads okay. Updated here 1118b3e

I removed the "for this job" part...

Should it be removed from the UI text on the Advanced Settings page too?

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

jgowdyelastic · 2020-09-02T15:21:39Z

x-pack/plugins/ml/public/application/overview/components/anomaly_detection_panel/actions.tsx

 import { MlSummaryJobs } from '../../../../../common/types/anomaly_detection_jobs';
+import { useCreateADLinks } from '../../../components/custom_hooks/use_create_ad_links';
+
+// @ts-ignore no module file


looks like this comment can go

Removed here 1118b3e

jgowdyelastic · 2020-09-02T15:32:24Z

x-pack/plugins/ml/server/lib/register_settings.ts

+        defaultMessage: 'Time filter defaults',
+      }),
+      type: 'json',
+      value: `{


is it worth pulling these defaults out into constants like our existing file data viz setting.
this could be stored as a real json object constant and stringified when adding it here.
JSON.stringify(FOO, null, 2) for the correct formatting.

This is super neat! Updated here 1118b3e

Although I put the new settings in the settings constants instead of creating a new file because I didn't think it was big enough. Although I'm happy to create a new file for them!

…settings

lcawl · 2020-09-02T20:25:19Z

x-pack/plugins/ml/common/constants/settings.ts

@@ -5,3 +5,11 @@
 */

 export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
+export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetection:results:enableTmeDefaults';


I think there's a letter missing:

Suggested change

export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetection:results:enableTmeDefaults';

export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetection:results:enableTimeDefaults';

Thanks! Fixed in ab8b130

peteharverson

Tested latest edits and LGTM

…settings

jgowdyelastic

LGTM

kibanamachine · 2020-09-04T16:20:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 903f660

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
ml	1386	+1	1385

async chunks size

id	value	diff	baseline
ml	8.2MB	+5.9KB	8.2MB

page load bundle size

id	value	diff	baseline
ml	584.2KB	+1.5KB	582.7KB

History

💚 Build #72283 succeeded ab8b130
💛 Build #71976 was flaky 4272114
💚 Build #71945 succeeded 24ad0d0
💚 Build #71725 succeeded 41af491
💔 Build #71480 failed 911ad3f

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

… for AD jobs (elastic#76347)

…filter for AD jobs (#76347) (#76798)

* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...

qn895 added 3 commits August 31, 2020 18:20

[ML] Add time range settings to ml advanced settings

86bd765

[ML] Add change so list of jobs reflect default settings

d92932d

[ML] Reorder settings order & remove console.log

ee5e375

qn895 added :ml Feature:Anomaly Detection ML anomaly detection labels Sep 1, 2020

qn895 self-assigned this Sep 1, 2020

[ML] Reorder settings order to have verb after setting name

911ad3f

qn895 commented Sep 1, 2020

View reviewed changes

lcawl mentioned this pull request Sep 1, 2020

[DOCS] Add default time range filter to advanced settings #76414

Merged

1 task

elastic deleted a comment from qn895 Sep 1, 2020

lcawl reviewed Sep 1, 2020

View reviewed changes

[ML] Create new hook to create AD links

6b35e20

lcawl reviewed Sep 1, 2020

View reviewed changes

qn895 added 4 commits September 1, 2020 14:50

[ML] Add toast notification for Anomaly Explorer

ff450a5

[ML] Add toast notification for SMV

0176c59

[ML] Change to pass 'invalid' to url to retain the original full rang…

4155201

…e + update field values

[ML] Update texts

41af491

qn895 marked this pull request as ready for review September 1, 2020 21:26

qn895 requested a review from a team as a code owner September 1, 2020 21:26

qn895 requested review from jgowdyelastic, peteharverson and alvarezmelissa87 September 1, 2020 21:27

qn895 added release_note:enhancement v7.10.0 v8.0.0 labels Sep 1, 2020

peteharverson reviewed Sep 2, 2020

View reviewed changes

x-pack/plugins/ml/server/lib/register_settings.ts Outdated Show resolved Hide resolved

peteharverson reviewed Sep 2, 2020

View reviewed changes

[ML] Change namings

56bef02

jgowdyelastic reviewed Sep 2, 2020

View reviewed changes

qn895 added 3 commits September 2, 2020 11:18

[ML] Add constants, update descriptions

1118b3e

Merge remote-tracking branch 'upstream/master' into default-adv-time-…

24ad0d0

…settings

[ML] Rename timerange to time range

4272114

lcawl reviewed Sep 2, 2020

View reviewed changes

[ML] Update typo ml:anomalyDetection:results:enableTimeDefaults

ab8b130

peteharverson approved these changes Sep 3, 2020

View reviewed changes

qn895 added 2 commits September 4, 2020 09:18

Merge remote-tracking branch 'upstream/master' into default-adv-time-…

3d938ac

…settings

[ML] Update width for group control checkbox column

903f660

jgowdyelastic approved these changes Sep 4, 2020

View reviewed changes

qn895 merged commit ee3c8d0 into elastic:master Sep 4, 2020

qn895 deleted the default-adv-time-settings branch September 4, 2020 16:46

qn895 mentioned this pull request Sep 4, 2020

[7.x] [ML] Add option to Advanced Settings to set default time range filter for AD jobs (#76347) #76798

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Sep 4, 2020

[ML] Add option to Advanced Settings to set default time range filter…

c20a749

… for AD jobs (elastic#76347)

qn895 added a commit that referenced this pull request Sep 4, 2020

[7.x] [ML] Add option to Advanced Settings to set default time range …

bbf75f1

…filter for AD jobs (#76347) (#76798)

	export const ANOMALY_DETECTION_DEFAULT_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeSet';
	export const ANOMALY_DETECTION_DEFAULT_TIME_RANGE = 'ml:anomalyDetectionTimeDefaults';

	defaultMessage: 'Enable default time range for anomaly detection jobs.',
	defaultMessage: 'Enable time filter defaults',

	defaultMessage: 'Use a default time range to view anomaly detection jobs.',
	defaultMessage: 'Use the default time filter in the Single Metric Viewer and Anomaly Explorer.',

	defaultMessage: 'The default time range to view anomaly detection jobs.',
	defaultMessage: 'The time filter selection to use when viewing anomaly detection job results.',

	export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionDefaultTimeRangeEnable';
	export const ANOMALY_DETECTION_ENABLE_TIME_RANGE = 'ml:anomalyDetectionTimeDefaultsEnable';

[ML] Add option to Advanced Settings to set default time range filter for AD jobs #76347

[ML] Add option to Advanced Settings to set default time range filter for AD jobs #76347

Conversation

qn895 commented Sep 1, 2020 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

peteharverson commented Sep 1, 2020

Choose a reason for hiding this comment

lcawl 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

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 Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

qn895 commented Sep 1, 2020 • edited by peteharverson Loading

elasticmachine commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qn895 Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson 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

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

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

qn895 commented Sep 1, 2020 •

edited

Loading

qn895 Sep 1, 2020 •

edited

Loading

qn895 commented Sep 1, 2020 •

edited by peteharverson

Loading

qn895 Sep 2, 2020 •

edited

Loading