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] Do not match time series counter fields with aggs in wizards #153021

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Mar 9, 2023

Time series counter metric fields are treated as non aggregatable and are not matched with aggregations in the new_job_caps endpoint.
This removes them from the detector dropdowns in all wizards where we match functions(aggs) to fields.
e.g.
image

Note, the fields are not entirely removed from the new_job_caps response. So they are still available in other dropdowns.

This fixes an issue where having counter fields available for selection would cause an error.

@jgowdyelastic jgowdyelastic mentioned this pull request Mar 9, 2023
4 tasks
@jgowdyelastic jgowdyelastic self-assigned this Mar 9, 2023
@jgowdyelastic jgowdyelastic marked this pull request as ready for review March 9, 2023 15:25
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner March 9, 2023 15:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@droberts195 droberts195 changed the title [ML] Dot not match time series counter fields with aggs in wizards [ML] Do not match time series counter fields with aggs in wizards Mar 9, 2023
@droberts195
Copy link
Contributor

It might be possible to do something better than this for 8.8. elastic/elasticsearch#93884 is adding a list of supported aggregations. There's a CCS complication but a suggestion of how to deal with that in elastic/elasticsearch#93884 (comment).

Depending on the resolution of the CCS issue we should do one of the following:

  • Merge this PR for 8.8, then do something better in 8.9 by making use of the list of available aggregations per field
  • Make use of the list of available aggregations per field in 8.8, assuming something sensible is being returned in the case of CCS against 8.7 clusters

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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 3.4MB 3.4MB +32.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @jgowdyelastic

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 LGTM.

As @droberts195 suggested, once elastic/elasticsearch#93884 is in, we should look to show these field types and check the available aggregations.

@jgowdyelastic jgowdyelastic merged commit 28a70da into elastic:main Mar 22, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 22, 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 Feature:Data Frame Analytics ML data frame analytics features :ml release_note:fix v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants