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

[Logs UI] Provide index name pattern choice during ML job setup #48231

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Oct 15, 2019

Summary

This fixes #48219 by adding the option for the user to select a subset of the configured log indices during the setup process. It also surfaces the errors returned by Kibana when the setup fails.

Description

The subset is derived by splitting the configured log index name pattern on commas and presenting the in a checkbox group. That split tries to emulate what happens to the index setting in ML datafeeds upon start.

A naive validation checks that at least one sub-pattern is selected, but it doesn't check for validity of the individual patterns.

This change also required a change to the logic of how outdated job configurations are detected. Instead of an exact match, a job configuration is now considered outdated if any of the sub-patterns used during deployment is not part of the log indices set in the source configuration anymore.

The errors returned by the job setup API are now presented in error call-outs, because they give hints as to the exact pattern that caused the deployment to fail.

Previews

grafik

grafik

grafik

Checklist

@weltenwort weltenwort added v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 15, 2019
@weltenwort weltenwort self-assigned this Oct 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui (Team:infra-logs-ui)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the logs-ui-analysis-add-index-pattern-setup-choice branch from 8cb973d to 1b003d7 Compare October 15, 2019 16:27
@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the logs-ui-analysis-add-index-pattern-setup-choice branch from 1b003d7 to 30a1255 Compare October 15, 2019 19:27
@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the logs-ui-analysis-add-index-pattern-setup-choice branch from 30a1255 to 16b595c Compare October 16, 2019 11:49
@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the logs-ui-analysis-add-index-pattern-setup-choice branch from 16b595c to f3aa009 Compare October 16, 2019 13:47
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@weltenwort weltenwort marked this pull request as ready for review October 17, 2019 13:12
@weltenwort weltenwort requested a review from a team as a code owner October 17, 2019 13:12
@afgomez
Copy link
Contributor

afgomez commented Oct 17, 2019

A nitpicky comment: should we prevent the form from submitting if the user hasn't selected any indices?

Screenshot 2019-10-17 at 17 00 47

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Nice work! I did add some comments on small details I found on a first pass. I also found a small nitpicky detail when playing with the UI (see comment above), but overall looks good.

Please confirm if the comments I proposed make sense. Otherwise I can approve straight away

@weltenwort
Copy link
Member Author

Thanks for the quick review. That all makes a lot of sense - I'll tackle it next thing in the morning 👍

@jasonrhodes
Copy link
Member

@weltenwort just wanted to say that the screenshots here look really good, I'm super happy with this "best we can do" solution to this onboarding problem.

@sgrodzicki
Copy link

I just wanted to comment on that as I was struggling with setting up ML jobs with sample data only as filebeat-* indices were missing and this change solved the problem for me. Great job @weltenwort!

Before:
Screenshot 2019-10-18 at 13 01 25

After:
Screenshot 2019-10-18 at 13 30 30

@weltenwort
Copy link
Member Author

@afgomez I made some changes based on your feedback:

  • The spelling for "analyze" is now consistent with American English.
  • The validation logic is simpler.
  • The (re)creation button is now disabled if the form is not deemed valid.
  • The file structure of the setup section now matches the import direction (downwards).
  • The ValidationError type is shared between UI components, but not with the container. The reasoning is that they represent the provider and the consumer side, the types of which must be compatible, but are not "the same". One declares that it provides and the other declares what it consumes.

Let me know what you think.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@afgomez
Copy link
Contributor

afgomez commented Oct 21, 2019

@weltenwort thanks!

The ValidationError type is shared between UI components, but not with the container.

Makes sense. If for whatever reason this hurts in the future (i.e. we need to add more error types and we see too much duplication) we can find a different way.

I'll approve

@weltenwort
Copy link
Member Author

@elasticmachine update branch

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350
Copy link
Contributor

(LGTM 👌)

@weltenwort weltenwort merged commit 603e27e into elastic:master Oct 21, 2019
@weltenwort weltenwort deleted the logs-ui-analysis-add-index-pattern-setup-choice branch October 21, 2019 23:20
weltenwort added a commit to weltenwort/kibana that referenced this pull request Oct 21, 2019
…tic#48231)

This fixes elastic#48219 by adding the option for the user to select a subset of the configured log indices during the setup process. It also surfaces the errors returned by Kibana when the setup fails.
weltenwort added a commit to weltenwort/kibana that referenced this pull request Oct 21, 2019
…tic#48231)

This fixes elastic#48219 by adding the option for the user to select a subset of the configured log indices during the setup process. It also surfaces the errors returned by Kibana when the setup fails.
weltenwort added a commit that referenced this pull request Oct 22, 2019
Backports the following commits to 7.x:
 - [Logs UI] Provide index name pattern choice during ML job setup (#48231)
weltenwort added a commit that referenced this pull request Oct 22, 2019
Backports the following commits to 7.5:
 - [Logs UI] Provide index name pattern choice during ML job setup (#48231)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] ML job creation fails when any sub-pattern fails to match
6 participants