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

[RAM] Add error logs in rule details page #128925

Merged
merged 30 commits into from
Apr 2, 2022
Merged

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Mar 30, 2022

Summary

image

image

Checklist

@XavierM XavierM added bug Fixes for quality problems that affect the customer experience UX impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 30, 2022
@XavierM XavierM requested a review from a team as a code owner March 30, 2022 15:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@XavierM XavierM added v8.2.0 v8.3.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement labels Mar 30, 2022
Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

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

I tested the error logs locally, everything looks good, other than the comments that I left

I'll let someone else give the green light for the snooze section.

onRuleChanged={requestRefresh}
direction="row"
isEditable={hasEditButton}
previousSnoozeInterval={null}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to specifically pass in null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because we are not going to backport this feature for 8.2, so I will have to remove it. But I think that we should also move the code out of the rule list page to RuleStatusDropdown, therefore we are not duplicating this functionality everywhere we used this component. We will follow up with a new PR.

sorting={{ sort }}
pagination={pagination}
onChange={({ page: changedPage, sort: changedSort }) => {
if (changedPage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not really a big deal, but should we break this into onChangePage and onChangeSort functions?

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Error log table looks good! Left some comments about the updated error/warning banners.

@XavierM XavierM requested a review from a team as a code owner April 1, 2022 20:02
Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

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

LGTM, but just wanted to make sure that we are only disabling the last response, and not the average duration or execution duration graph

Screenshot from 2022-04-01 13-09-19

Also do we want a more detailed wording for the disabled messaging? Since the old messaging explained why the rule summary couldn't be display, this just says "disabled"

@XavierM
Copy link
Contributor Author

XavierM commented Apr 1, 2022

@JiaweiWu I worked with @ryankeairns and here we go
image

@cchaos cchaos dismissed their stale review April 1, 2022 20:56

Issue was fixed, but didn't do a review.

@ryankeairns
Copy link
Contributor

Design PR

Merge at your leisure:
👉 XavierM#10


Screen Shot 2022-04-01 at 4 36 33 PM

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Just some comments about the types and the sorting issue discussed offline and this LGTM!

@XavierM XavierM enabled auto-merge (squash) April 2, 2022 01:45
@XavierM XavierM merged commit d6b8e4b into elastic:main Apr 2, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #1 / APM API tests trial apm_mappings_only_8.0.0 fetching service anomalies with a trial license checks if alert is active

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 358 359 +1

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
alerting 317 326 +9

Async chunks

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

id before after diff
triggersActionsUi 700.3KB 697.7KB -2.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 21 20 -1

Page load bundle

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

id before after diff
triggersActionsUi 98.6KB 98.6KB -4.0B
Unknown metric groups

API count

id before after diff
alerting 325 334 +9

async chunk count

id before after diff
triggersActionsUi 73 75 +2

ESLint disabled line counts

id before after diff
triggersActionsUi 160 164 +4

Total ESLint disabled count

id before after diff
triggersActionsUi 166 170 +4

History

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

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.2 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.2:
- [RAM] Disable rule status dropdown on readonly user (#128971)

Manual backport

To create the backport manually run:

node scripts/backport --pr 128925

Questions ?

Please refer to the Backport tool documentation

XavierM added a commit to XavierM/kibana that referenced this pull request Apr 3, 2022
* add error log on details page + snooze

* refresh table in details page

* fix type

* add + fix jest test

* add test for error log

* remove console

* update functional test + delete flaky jest test + fix i18n

* review I

* remove skip

* review II

* remove disable panel

* clean up design

* fix check

* fix types

* Design tweaks to header, status dropdown, mobile

* remove dead code

* jest test

* fix actions layout

* review III

Co-authored-by: Ryan Keairns <[email protected]>
(cherry picked from commit d6b8e4b)
XavierM added a commit that referenced this pull request Apr 3, 2022
* add error log on details page + snooze

* refresh table in details page

* fix type

* add + fix jest test

* add test for error log

* remove console

* update functional test + delete flaky jest test + fix i18n

* review I

* remove skip

* review II

* remove disable panel

* clean up design

* fix check

* fix types

* Design tweaks to header, status dropdown, mobile

* remove dead code

* jest test

* fix actions layout

* review III

Co-authored-by: Ryan Keairns <[email protected]>
(cherry picked from commit d6b8e4b)
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 bug Fixes for quality problems that affect the customer experience impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) UX v8.2.0 v8.3.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

8 participants