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.

Sign up for GitHub

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

Jump to bottom

[Logs UI] Fix initial selection of log threshold alert condition field if missing from mapping #86488

Merged
merged 10 commits into from
Dec 22, 2020
Merged

[Logs UI] Fix initial selection of log threshold alert condition field if missing from mapping #86488

merged 10 commits into from
Dec 22, 2020

Conversation

weltenwort
Copy link
Member

Summary

This avoid selecting the log.level field as the default in log threshold alerts if it is not present in the mapping of at least one source index.

fixes #84414

Previews

image

image

image

Implementation notes

While following the code paths leading to the error I noticed the assumptions about the prop types passed to the alert expression editor are unvalidated. They were not tied to the actual prop types and therefore didn't cause the compiler to warn. By using the upstream alerting types in combination with an io-ts decoder this tries to make sure the types are sound both at compile- and runtime.

@weltenwort weltenwort added bug Fixes for quality problems that affect the customer experience 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 v7.11.0 v7.12.0 labels Dec 18, 2020
@weltenwort weltenwort added this to the Logs UI 7.11 milestone Dec 18, 2020
@weltenwort weltenwort self-assigned this Dec 18, 2020
@weltenwort weltenwort marked this pull request as ready for review December 18, 2020 23:36
@weltenwort weltenwort requested a review from a team as a code owner December 18, 2020 23:36
@elasticmachine
Copy link
Contributor

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

@afgomez afgomez self-requested a review December 21, 2020 11:25
@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

afgomez
afgomez approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@afgomez afgomez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tried locally with the kibana_sample_logs index and it works as intended.

I have one observation: the first state, when no field is selected, looks "broken". As a user I wasn't sure what I was supposed to do, only that there was some invisible error after "WITH". Maybe it makes sense to pre-select the first field to show... something?

Other than that LGTM!

@weltenwort
Copy link
Member Author

weltenwort commented Dec 21, 2020 •
edited
Loading

Yeah, I'm unsure about that. While selecting the first available field wouldn't be too hard, coming up with a valid default value would be some effort. It would have to be different for the different types (text vs number vs ip, etc.). I could also select the first field and leave the value empty, but that would still be marked as invalid. Would that be an improvement?

@weltenwort
Copy link
Member Author

💭 Or we could render "WITH a chosen field" if no field is selected by default. Would that imply that the user should choose a field?

@weltenwort
Copy link
Member Author

@afgomez what do you think about this?

image

@weltenwort
Copy link
Member Author

I noticed that I forgot to handle the "add new condition" case when no log.level field exists:

image

I'll push a fix promptly.

@weltenwort weltenwort requested a review from afgomez December 21, 2020 19:47
@weltenwort
Copy link
Member Author

@afgomez given the various changes, would you mind having another look?

afgomez
afgomez approved these changes Dec 22, 2020
View reviewed changes
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.

Still LGTM :) Thanks for adding the extra sentence for the empty field case 🙇

@kibanamachine
Copy link
Contributor

💚 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
infra 2.7MB 2.7MB +1.8KB

Distributable file count

id before after diff
default 47144 47904 +760

Page load bundle

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

id before after diff
infra 179.8KB 183.1KB +3.3KB

History

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

@weltenwort weltenwort merged commit 24db3a0 into elastic:master Dec 22, 2020
@weltenwort weltenwort deleted the logs-ui-fix-alert-condition-initial-field-selection branch December 22, 2020 18:36
This was referenced Dec 22, 2020
Merged
Merged
weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 22, 2020
@weltenwort
[Logs UI] Fix initial selection of log threshold alert condition fiel…
b55b240 
…d if missing from mapping (elastic#86488)

This avoid selecting the `log.level` field as the default in log threshold alerts if it is not present in the mapping of at least one source index.
weltenwort added a commit that referenced this pull request Dec 23, 2020
@weltenwort
[7.11] [Logs UI] Fix initial selection of log threshold alert conditi…
b4851bb 
…on field if missing from mapping (#86488) (#86820)

Backports the following commits to 7.11:
 - [Logs UI] Fix initial selection of log threshold alert condition field if missing from mapping (#86488)
weltenwort added a commit that referenced this pull request Dec 23, 2020
@weltenwort
[7.x] [Logs UI] Fix initial selection of log threshold alert conditio…
46f4c7c 
…n field if missing from mapping (#86488) (#86819)

Backports the following commits to 7.x:
 - [Logs UI] Fix initial selection of log threshold alert condition field if missing from mapping (#86488)
@weltenwort weltenwort mentioned this pull request Jan 6, 2021
Merged
weltenwort added a commit that referenced this pull request Jan 7, 2021
@weltenwort
[Logs UI] Fix the ability to switch to the ratio alert type (#87563)
f170ce2 
This fixes a bug introduced with #86488, which prevents the user from changing the log threshold alert type to `ratio`.
weltenwort added a commit to weltenwort/kibana that referenced this pull request Jan 7, 2021
@weltenwort
[Logs UI] Fix the ability to switch to the ratio alert type (elastic#…
350d067 
…87563)

This fixes a bug introduced with elastic#86488, which prevents the user from changing the log threshold alert type to `ratio`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afgomez afgomez afgomez approved these changes

Assignees

@weltenwort weltenwort

Labels
bug Fixes for quality problems that affect the customer experience Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.11.0 v7.12.0 v8.0.0
Projects
None yet
Milestone
Logs UI 7.11
Development

Successfully merging this pull request may close these issues.

First field cannot be selected in Alerts and Actions Log treshold alert
4 participants
@weltenwort @elasticmachine @kibanamachine @afgomez