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] Creates categorization job from pattern analysis #170567

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Nov 3, 2023

Adds the ability to quickly create a categorisation anomaly detection job from the pattern analysis flyout.
Adds a new created_by ID categorization-wizard-from-pattern-analysis which can be picked up by telemetry.

Creates a new package for sharing our AIOPs ui actions IDs. I think we should move the pattern analysis ID to this package too, but that can be done in a separate PR.

2023-11-09.17-13-03.2023-11-09.17_13_54.mp4
2023-11-09.17-27-29.2023-11-09.17_28_10.mp4

@jgowdyelastic jgowdyelastic added :ml Feature:Anomaly Detection ML anomaly detection release_note:feature Makes this part of the condensed release notes v8.12.0 labels Nov 13, 2023
@jgowdyelastic jgowdyelastic marked this pull request as ready for review November 13, 2023 13:46
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner November 13, 2023 13:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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 and overall looks good. Just one question mark around adding an influencer for per-partition jobs.

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

<>
<EuiButtonEmpty
data-test-subj="aiopsLogCategorizationFlyoutAdJobButton"
onClick={() => createADJob()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just pass in createADJob here without creating a wrapper function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in d32017d

try {
categorizationType = rison.decode(categorizationTypeRisonString) as CategorizationType;
} catch (error) {
categorizationType = CATEGORIZATION_TYPE.COUNT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there's a pattern of catching and setting the value to an empty string - maybe it would be good to have a small util function that abstracts that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, these can be cleaned up quite a lot.
Updated in d32017d
I've also changed the maps and lens versions of this file.

) {
const locator = share.url.locators.get(ML_APP_LOCATOR);

const url = await locator?.getUrl({
Copy link
Contributor

Choose a reason for hiding this comment

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

Does there need to be a contingency for when locator is undefined? So the url doesn't end up as undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's safe to assume that locator will alway be defined as we're in the ML app. We do this in other places.
I've updated the code to assert that locator is defined.
Updated in d32017d and 055a53b

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Left a few comments but overall LGTM
Giving it the green checkmark to unblock

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 416 419 +3
ml 1846 1859 +13
total +16

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ml-ui-actions - 7 +7

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 375.8KB 376.5KB +751.0B
ml 3.6MB 3.6MB +19.3KB
total +20.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 78.6KB 78.7KB +148.0B
Unknown metric groups

API count

id before after diff
@kbn/ml-ui-actions - 7 +7
aiops 69 70 +1
total +8

async chunk count

id before after diff
ml 35 36 +1

ESLint disabled line counts

id before after diff
ml 557 559 +2

Total ESLint disabled count

id before after diff
ml 560 562 +2

History

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

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic merged commit 5e3b124 into elastic:main Nov 21, 2023
29 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 21, 2023
@szabosteve szabosteve changed the title [ML] Create categorization job from pattern analysis [ML] Creates categorization job from pattern analysis Dec 8, 2023
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:feature Makes this part of the condensed release notes v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants