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

[RAC] Updates Alerts table cell actions #116446

Merged
merged 10 commits into from
Nov 2, 2021

Conversation

claudiopro
Copy link
Contributor

@claudiopro claudiopro commented Oct 27, 2021

Summary

This PR fixes #111090 by

  • Relabeling the Filter for value action to Filter in
  • Adding a new Filter out action that excludes the given value from the query
    • This change is temporarily put on hold after discussion with @mgiota
  • Removes cell actions from the Duration and Reason fields
  • Removes the Copy to clipboard action from all cells

Screen Shot 2021-10-27 at 16 01 32

Screen Shot 2021-10-28 at 12 30 08

Screen Shot 2021-10-28 at 15 47 59

I'm also tweaking the label text to use sentence-style capitalization as recommended by EUI writing guidelines.

Known issues

  • Clicking Filter in multiple times on the same value keeps adding to the query - this is already occurring in main and it is not regression.
  • Filtering in and then out for the same value in this sequence creates an empty result set, while I assume filtering out should remove the previous filter instead.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@claudiopro
Copy link
Contributor Author

claudiopro commented Oct 27, 2021

Looks like the test ObservabilityApp Observability alerts Alerts table Cell actions "before each" hook for "Copy button works" is failing after the removal of the Copy buttton. I'll address it tomorrow.

The other test failure Uptime app with generated data When on the Synthetics Integration Policy Create Page create new policy allows saving when user enters a valid integration name and url/host doesn't seem related to this PR, maybe a flaky test? A rebase will hopefully address this.

@claudiopro claudiopro force-pushed the 111090_RAC_update_cell_actions branch from c0a459d to 44462c6 Compare October 27, 2021 20:19
@mgiota
Copy link
Contributor

mgiota commented Oct 28, 2021

@claudiopro Good job! I'll put some comments directly in the line numbers.

Regarding failing tests yep uptime tests look irrelevant, so to update your PR just type @elasticmachine merge upstream in a comment (not sure if me typing this will re-run the PR, but in general you write above command and your PR re-runs or gets updated with latest master)

Regarding copy button failing tests, here's how you run them locally.

You first run the sever

node scripts/functional_tests_server --config x-pack/test/observability_functional/with_rac_write.config.ts

And then you run the tests:

node scripts/functional_test_runner --config x-pack/test/observability_functional/with_rac_write.config.ts --grep='Observability alerts'

(you can grep accordingly)

In order to link a PR with a Github issue you need to write fixes #issue_number. I updated your description accordingly (I renamed addresses to fixes)

@mgiota
Copy link
Contributor

mgiota commented Oct 28, 2021

@claudiopro according to ACs duration should have only the expand icon. We should remove the filter in/out options for the duration column.

I want to comment regarding the issue you described when user filters in and then filters out the same value. I recommend we completely remove the Filter out functionality and properly implement it as part of this issue #116135, because now it looks broken and confusing. cc @katrin-freihofner @hbharding

@mgiota
Copy link
Contributor

mgiota commented Oct 28, 2021

@claudiopro Here's an example of how you should label your PR #115171 once you open it for review

@claudiopro claudiopro force-pushed the 111090_RAC_update_cell_actions branch 2 times, most recently from af0ddde to d9ab1d2 Compare October 28, 2021 13:45
@claudiopro claudiopro force-pushed the 111090_RAC_update_cell_actions branch from d9ab1d2 to 7cbc1c2 Compare October 28, 2021 13:45
@claudiopro
Copy link
Contributor Author

@mgiota postponing the implementation of the Filter out action until we implement the filter manager seems reasonable to me. After acquiring more context, I agree we should avoid shipping a brittle implementation.

@claudiopro
Copy link
Contributor Author

@elasticmachine merge upstream

@claudiopro claudiopro marked this pull request as ready for review October 28, 2021 22:50
@claudiopro claudiopro requested a review from a team as a code owner October 28, 2021 22:50
@claudiopro claudiopro added release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.0.0 Theme: rac label obsolete labels Oct 28, 2021
@mgiota mgiota self-requested a review October 29, 2021 07:47
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

LGTM!

@claudiopro
Copy link
Contributor Author

@elasticmachine merge upstream

@claudiopro claudiopro added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 2, 2021
@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
observability 376.0KB 375.6KB -481.0B
timelines 235.9KB 236.0KB +49.0B
total -432.0B

History

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

@claudiopro claudiopro merged commit c149fe6 into elastic:main Nov 2, 2021
@claudiopro claudiopro deleted the 111090_RAC_update_cell_actions branch November 2, 2021 16:43
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 2, 2021
* Adds Filter Out button to alert table cell flyout

* Adds translations

* Fixes capitalization of labels

* Removes unused declarations and imports

* Fixes and adds functional tests for Alerts table action buttons

* Addresses review comments

* Fixes Alert table cell actions functional tests

* Removes Filter out action for now

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 2, 2021
* Adds Filter Out button to alert table cell flyout

* Adds translations

* Fixes capitalization of labels

* Removes unused declarations and imports

* Fixes and adds functional tests for Alerts table action buttons

* Addresses review comments

* Fixes Alert table cell actions functional tests

* Removes Filter out action for now

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Claudio Procida <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Theme: rac label obsolete v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAC] Observability alerts table: update cell actions
6 participants