Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Quickly create ML jobs from lens visualizations #136421

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jul 14, 2022

Anomaly detection jobs can now be created instantly from the flyout within the dashboard app.
The original option to create via the wizard is also still available as a link to the ML app.

9C18D861-C981-4D3D-BFC6-54079C455ED4-12792-0000AC8CE03A7D74

@jgowdyelastic jgowdyelastic self-assigned this Jul 14, 2022
@jgowdyelastic jgowdyelastic marked this pull request as ready for review August 10, 2022 15:00
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner August 10, 2022 15:00
@jgowdyelastic jgowdyelastic changed the title [ML] Quickly create ML jobs from lens visualizations [ML] [WIP] Quickly create ML jobs from lens visualizations Aug 10, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic jgowdyelastic marked this pull request as draft August 11, 2022 08:52
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest changes and LGTM

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Aug 25, 2022

@elastic/kibana-design you have been pinged because I have deleted a scss file.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with refactoring, overall LGTM! Left some minor suggestions

startJob: boolean,
runInRealTime: boolean,
layerIndex: number,
mlApiServices: MlApiServices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I see it's only been used by createAndSaveJob method, but maybe it's worth passing to a constructor instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the last refactor I moved mlApiServices to the context after changing this, so possibly yes.
Previously it wasn't available in all of the locations QuickJobCreator was instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in 336db0c

// add job config and start and end dates to the
// job cloning stash, so they can be used
// by the new job wizards
stashJobForCloning(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the content of this function an noticed it utilizes mlJobService which is statically exported. As an idea for further enhancements: I reckon we should provide this service from the plugin contract setup, so the other plugins can consume it as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mlJobService is very old code and shouldn't be shared.
Most of it has been replaced by other, better, services. I believe it is only the results pages which still use it.

The most useful part is the job stash used for cloning. So ideally we should get rid of mlJobService and move the cloning functionality to a separate service.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ do we want to show an error toast here?

Copy link
Member Author

@jgowdyelastic jgowdyelastic Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is ok to fail silently here.
includeTimeRange is set to false which means the wizard will not auto set the time. The user can then choose one.

},
});

window.open(url, '_blank');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use navigateToUrl from CoreStart['application']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link needs to open in a new tab, from what I can tell navigateToUrl does not allow this as an option.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
One bit of UI feedback would be to change the background of the panel in the flyout to white (where you define the job settings). This will help contrast it against the gray background of the flyout.
Was curious if it also made sense to add in the panel title for more context? Instead of only the layer type?

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Aug 25, 2022

Thanks @mdefazio , I've added the visualisation title to the description at the top of the flyout. 65289fd

image

Regarding the background colour, I copied the style used in Lens:
image

Changing the colour in my panels does make it standout from the background, but IMO they look a bit too white and strange.

image

vs the original

image

@mdefazio
Copy link
Contributor

Thanks @jgowdyelastic , is there a scenario where you're setting up 2+ jobs?

The one difference between Lens is that the input or area of focus is still on white (@timestamp or Median of CPUtilization is in a white panel over the gray). Granted, because you're including the input field directly in place of the inner panel, it's a bit up in the air as to how to present this. But simply for contrast on the input, the white might be worth it.

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Aug 25, 2022

@mdefazio

is there a scenario where you're setting up 2+ jobs?

If you have multiple compatible layers in a visualization it would be possible to create multiple jobs from a single flyout. The layer components are independent from each other.
It's worth noting, in case there is come confusion here, that the layers themselves are not named. The best I can do is use the layer types as the title (e.g. stacked bar chart)
This was raised before in the previous PR #129762 (comment) (second half of this comment). It would be better to have a more descriptive title for the layer, but it's not easy, or even possible.

image

@mdefazio
Copy link
Contributor

Ok...now it's coming back to me! Thanks for the reminder of the history / context!

@jgowdyelastic jgowdyelastic enabled auto-merge (squash) August 26, 2022 06:49
@jgowdyelastic jgowdyelastic merged commit 4f65c09 into elastic:main Aug 26, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #27 / a11y tests using flights sample data "after all" hook in "using flights sample data"
  • [job] [logs] FTR Configs #27 / a11y tests using flights sample data "before all" hook in "using flights sample data"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1606 1605 -1

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.3MB 3.3MB +9.4KB
Unknown metric groups

ESLint disabled line counts

id before after diff
ml 98 101 +3

Total ESLint disabled count

id before after diff
ml 101 104 +3

History

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

cc @jgowdyelastic

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 26, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* [ML] Quickly create ML jobs from lens visualizations

* adding additional settings

* adding model plot for single metric jobs

* adding results link

* form logic improvements

* translations

* translations

* translations

* changing view results link generation

* handling opening already open jobs

* fixing count detector

* removing unused file

* improving error displaying

* updating tests

* removing unused test code

* deleting jobs after test

* code clean up

* fixing issue with layers sharing resources

* removing fragment

* moving code about

* minor changes based on review

* translations

* updating results link for non-running job

* renaming variables

* refactoring job creation code into class

* using context refactor

* renaming things

* catching isCompatible errors

* fixing behaviour in canvas

* removing comment

* small refactor moving code

* handling error

* fixing typo

* further refactoring based on review

* adding vis title to description

* white layer panels

* translations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Anomaly Detection ML anomaly detection :ml release_note:enhancement v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants