[ML] Anomaly Detection: adds ability to delete forecasts from job #194896

alvarezmelissa87 · 2024-10-04T05:02:50Z

Summary

Related issues:

In this PR, in Job management > expanded row > Forecasts tab - a delete action has been added to each row in the forecasts table. A confirmation modal allows the user to confirm the delete action.

In the SMV view, the forecast being currently viewed is now highlighted in the Forecast modal to make it easier to identify.

Dark mode:

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

alvarezmelissa87 · 2024-10-04T05:04:43Z

cc @joana-cps - wanted to get your opinion on the color to highlight the selected row in the modal.

elasticmachine · 2024-10-04T11:43:39Z

Pinging @elastic/ml-ui (:ml)

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

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

...gins/ml/public/application/timeseriesexplorer/components/forecasting_modal/forecasts_list.js

peteharverson · 2024-10-04T12:14:02Z

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

@@ -1081,6 +1081,7 @@ export class TimeSeriesExplorer extends React.Component {
                  latestRecordTimestamp={selectedJob.data_counts.latest_record_timestamp}
                  setForecastId={this.setForecastId}
                  className="forecast-controls"
+                  selectedForecastId={this.props.selectedForecastId}


Are you clearing this when switching jobs from within the SMV? I am seeing the view try to render what I guess is the previous forecast here:

The selected id passed into the modal is only used in row props to highlight the selected forecast 🤔
Not sure what's going on here but I wasn't able to reproduce anything like that. Would you be up for sharing steps?

I can no longer reproduce this!

Not quite the same, but possibly related, on returning to a job that was previously displaying a forecast, the forecast modal is incorrectly implying that the previous forecast is being viewed:

Screen.Recording.2024-10-07.at.10.04.26.mov

I was able to reproduce that. Looks like the forecastId sent as a prop to child components was not getting reset when job selection changed. This has been fixed in c16e025
I gave it an extra test after and it looks like no other behavior is affected by that fix but would appreciate a second test. 🙏

Behavior when switching jobs / forecasts all looks good to me.

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

alvarezmelissa87 · 2024-10-04T19:10:32Z

@elasticmachine merge upstream

kibana-ci · 2024-10-04T21:52:17Z

💔 Build Failed

Failed CI Steps

Quick Checks

History

💔 Build #239584 failed 94680e2
💔 Build #239319 failed f1d9293

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

cc @alvarezmelissa87

jgowdyelastic · 2024-10-07T09:12:55Z

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

@@ -368,6 +368,14 @@ export function mlApiProvider(httpService: HttpService) {
      });
    },

+    deleteForecast({ jobId, forecastId }: { jobId: string; forecastId: string }) {
+      return httpService.http<any>({


It would be good to add the correct return type here

Updated in d29de18

jgowdyelastic · 2024-10-07T09:14:41Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

@@ -54,6 +89,11 @@ export class ForecastsTable extends Component {
  static contextType = context;

  componentDidMount() {
+    this.loadForecasts();
+    this.canDeleteJobForecast = checkPermission('canForecastJob');


we should add a new canDeleteForecast capability

Added in d29de18

szabosteve · 2024-10-07T12:01:28Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

+    <p>
+      <FormattedMessage
+        id="xpack.ml.jobsList.jobDetails.forecastsTable.deleteForecastConfirm.text"
+        defaultMessage="This will delete the forecast from the job permanently."


Nit.

Suggested change

defaultMessage="This will delete the forecast from the job permanently."

defaultMessage="This will permanently delete the forecast from the job."

Updated in d29de18

szabosteve · 2024-10-07T12:02:37Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

+        errorMessage: i18n.translate(
+          'xpack.ml.jobsList.jobDetails.forecastsTable.deleteForecastErrorMessage',
+          {
+            defaultMessage: 'An error occurred deleting the forecast.',


Nit.

Suggested change

defaultMessage: 'An error occurred deleting the forecast.',

defaultMessage: 'An error occurred when deleting the forecast.',

Updated in d29de18

joana-cps · 2024-10-07T12:24:45Z

cc @joana-cps - wanted to get your opinion on the color to highlight the selected row in the modal.

For consistency's sake, we should use the same blue we use in selected rows in other tables.

alvarezmelissa87 · 2024-10-07T19:42:35Z

@elasticmachine merge upstream

peteharverson

Tested latest changes and LGTM

joana-cps · 2024-10-08T14:29:15Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

+              defaultMessage: 'View',
+            }),
+            type: 'icon',
+            icon: 'singleMetricViewer',


As discussed on slack, can we change this one to the EUI:eye?

Suggested change

icon: 'singleMetricViewer',

icon: 'eye',

Sorry about that - updated in 79038f0

jgowdyelastic · 2024-10-08T15:26:53Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

@@ -44,6 +78,7 @@ export class ForecastsTable extends Component {
    this.state = {
      isLoading: props.job.data_counts.processed_record_count !== 0,
      forecasts: [],
+      isConfirmModalVisible: false,


Suggested change

isConfirmModalVisible: false,

isDeleteForecastConfirmModalVisible: false,

jgowdyelastic · 2024-10-08T15:30:27Z

.../public/application/jobs/jobs_list/components/job_details/forecasts_table/forecasts_table.js

+
+    this.setState({
+      isLoading: true,
+      isConfirmModalVisible: false,


forecastIdToDelete needs to be cleared here too and in the catch below.
Seeing as isConfirmModalVisible and forecastIdToDelete are always set together, I don't think we need both of them.
Whether or not the modal is shown could be based solely on whether forecastIdToDelete is defined.

Updated to use the forecastIdToDelete for modal visibility check and to clear in 0d7a3ab

jgowdyelastic · 2024-10-08T15:33:42Z

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

@@ -257,7 +258,7 @@ export const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateMan
        });
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [selectedForecastId]);
+  }, [selectedForecastId, selectedForecastId]);


selectedForecastId is repeated here. it didn't flag up because we're suppressing the exhaustive-deps error :(

Removed in 0d7a3ab

…-fix'

elasticmachine · 2024-10-08T18:50:50Z

💔 Build Failed

Buildkite Build
Commit: c1ec1b7

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #17 / discover/context_awareness extension getAdditionalCellActions data view mode should render additional cell actions for logs data source
[job] [logs] FTR Configs #51 / Machine Learning system ml_capabilities get capabilities should have the right number of capabilities
[job] [logs] FTR Configs #51 / Machine Learning system ml_capabilities get capabilities should have the right number of capabilities

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`	4.6MB	4.6MB	+2.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`ml`	103	104	+1

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.8KB	75.0KB	+152.0B

History

💔 Build #240025 failed dfcc270
💔 Build #240011 failed d29de18

cc @alvarezmelissa87

jgowdyelastic

LGTM

kibanamachine · 2024-10-09T08:41:18Z

Starting backport for target branches: 8.x

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

…astic#194896) ## Summary Related issues: - elastic#18511 - elastic#192301 In this PR, in Job management > expanded row > Forecasts tab - a delete action has been added to each row in the forecasts table. A confirmation modal allows the user to confirm the delete action. In the SMV view, the forecast being currently viewed is now highlighted in the Forecast modal to make it easier to identify. ![image](https://github.com/user-attachments/assets/87814889-d41d-4780-98ab-695c6f12a023) <img width="881" alt="image" src="https://github.com/user-attachments/assets/accbd7d9-1bae-4f1f-af8f-8bd36eae0572"> <img width="1099" alt="image" src="https://github.com/user-attachments/assets/6011936d-3773-41ce-bbce-3ca4c0154cab"> Dark mode: <img width="882" alt="image" src="https://github.com/user-attachments/assets/cbec6fc8-0c62-4e34-9546-0124ae80a568"> ### 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: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit f4a4a68)

kibanamachine · 2024-10-09T08:45:55Z

💚 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

…ob (#194896) (#195547) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Anomaly Detection: adds ability to delete forecasts from job (#194896)](#194896)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Melissa Alvarez <[email protected]>

alvarezmelissa87 added 2 commits October 2, 2024 10:34

wip: add delete action to forecast table

5efeea5

move delete functionality to expanded row job details

f1d9293

alvarezmelissa87 added release_note:enhancement backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.16.0 labels Oct 4, 2024

alvarezmelissa87 self-assigned this Oct 4, 2024

alvarezmelissa87 requested review from peteharverson, szabosteve and walterra October 4, 2024 05:03

alvarezmelissa87 marked this pull request as ready for review October 4, 2024 05:06

alvarezmelissa87 requested a review from a team as a code owner October 4, 2024 05:06

peteharverson added the :ml label Oct 4, 2024

peteharverson added the Feature:Anomaly Detection ML anomaly detection label Oct 4, 2024

peteharverson reviewed Oct 4, 2024

View reviewed changes

add permission check for delete action

e501a79

elasticmachine and others added 2 commits October 4, 2024 21:10

Merge branch 'main' into ml-forecast-deletion

94680e2

add api test for forecast deletion

e49f4c6

peteharverson mentioned this pull request Oct 7, 2024

[ML] Increase Test Coverage 8.16.0 #188459

Closed

23 tasks

jgowdyelastic reviewed Oct 7, 2024

View reviewed changes

szabosteve reviewed Oct 7, 2024

View reviewed changes

alvarezmelissa87 added 2 commits October 7, 2024 13:11

update delete copy and highlight color

d29de18

ensure forecastIdProp is updated correctly

c16e025

Merge branch 'main' into ml-forecast-deletion

dfcc270

peteharverson approved these changes Oct 8, 2024

View reviewed changes

joana-cps reviewed Oct 8, 2024

View reviewed changes

update forecast icon and remove unused translation

79038f0

jgowdyelastic reviewed Oct 8, 2024

View reviewed changes

kibanamachine and others added 3 commits October 8, 2024 15:53

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

b1bddcb

…-fix'

ensure forecast id to delete is reset

0d7a3ab

initialize forecastIdToDelete

c1ec1b7

update api integration capabilities test

fd653d7

jgowdyelastic approved these changes Oct 9, 2024

View reviewed changes

peteharverson added v9.0.0 backport:version Backport to applied version labels and removed backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Oct 9, 2024

peteharverson changed the title ~~[ML] Anomaly Detection: add ability to delete forecasts from job~~ [ML] Anomaly Detection: adds ability to delete forecasts from job Oct 9, 2024

peteharverson merged commit f4a4a68 into elastic:main Oct 9, 2024
25 checks passed

kibanamachine mentioned this pull request Oct 9, 2024

[8.x] [ML] Anomaly Detection: adds ability to delete forecasts from job (#194896) #195547

Merged

alvarezmelissa87 deleted the ml-forecast-deletion branch October 10, 2024 15:55

alvarezmelissa87 mentioned this pull request Oct 10, 2024

[ML] Add possibility to delete forecasts #18511

Closed

peteharverson mentioned this pull request Oct 11, 2024

[ML] Single metric viewer: Add support for deleting forecast in the UI #192301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Anomaly Detection: adds ability to delete forecasts from job #194896

[ML] Anomaly Detection: adds ability to delete forecasts from job #194896

alvarezmelissa87 commented Oct 4, 2024 •

edited by kibanamachine

Loading

alvarezmelissa87 commented Oct 4, 2024

elasticmachine commented Oct 4, 2024

peteharverson Oct 4, 2024

alvarezmelissa87 Oct 4, 2024

peteharverson Oct 7, 2024

peteharverson Oct 7, 2024

alvarezmelissa87 Oct 7, 2024

peteharverson Oct 8, 2024

alvarezmelissa87 commented Oct 4, 2024

kibana-ci commented Oct 4, 2024

jgowdyelastic Oct 7, 2024

alvarezmelissa87 Oct 7, 2024

jgowdyelastic Oct 7, 2024

alvarezmelissa87 Oct 7, 2024

szabosteve Oct 7, 2024

alvarezmelissa87 Oct 7, 2024

szabosteve Oct 7, 2024

alvarezmelissa87 Oct 7, 2024

joana-cps commented Oct 7, 2024

alvarezmelissa87 commented Oct 7, 2024

peteharverson left a comment

joana-cps Oct 8, 2024

joana-cps Oct 8, 2024

alvarezmelissa87 Oct 8, 2024

jgowdyelastic Oct 8, 2024

jgowdyelastic Oct 8, 2024 •

edited

Loading

alvarezmelissa87 Oct 8, 2024

jgowdyelastic Oct 8, 2024

alvarezmelissa87 Oct 8, 2024

elasticmachine commented Oct 8, 2024 •

edited

Loading

jgowdyelastic left a comment

kibanamachine commented Oct 9, 2024

kibanamachine commented Oct 9, 2024

	defaultMessage="This will delete the forecast from the job permanently."
	defaultMessage="This will permanently delete the forecast from the job."

	defaultMessage: 'An error occurred deleting the forecast.',
	defaultMessage: 'An error occurred when deleting the forecast.',

	isConfirmModalVisible: false,
	isDeleteForecastConfirmModalVisible: false,

[ML] Anomaly Detection: adds ability to delete forecasts from job #194896

[ML] Anomaly Detection: adds ability to delete forecasts from job #194896

Conversation

alvarezmelissa87 commented Oct 4, 2024 • edited by kibanamachine Loading

Summary

Checklist

alvarezmelissa87 commented Oct 4, 2024

elasticmachine commented Oct 4, 2024

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 Oct 4, 2024

kibana-ci commented Oct 4, 2024

💔 Build Failed

Failed CI Steps

History

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

joana-cps commented Oct 7, 2024

alvarezmelissa87 commented Oct 7, 2024

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

jgowdyelastic Oct 8, 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

elasticmachine commented Oct 8, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Public APIs missing exports

Page load bundle

History

jgowdyelastic left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 9, 2024

kibanamachine commented Oct 9, 2024

💚 All backports created successfully

Questions ?

alvarezmelissa87 commented Oct 4, 2024 •

edited by kibanamachine

Loading

jgowdyelastic Oct 8, 2024 •

edited

Loading

elasticmachine commented Oct 8, 2024 •

edited

Loading