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] Remove isSnoozeUntil and calculate on the fly for get/find #136148

Merged
merged 24 commits into from
Jul 26, 2022

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Jul 11, 2022

Summary

Refactor to avoid multiple call on the rule's SO and avoid some OCC's issue in the task runner
FIX: #135846 & #123584

Screen Shot 2022-07-26 at 5 43 53 AM

Screen Shot 2022-07-26 at 5 44 27 AM

Screen Shot 2022-07-26 at 5 44 49 AM

@XavierM XavierM added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.4.0 labels Jul 11, 2022
@XavierM
Copy link
Contributor Author

XavierM commented Jul 12, 2022

@elasticmachine merge upstream

@XavierM XavierM marked this pull request as ready for review July 12, 2022 20:44
@XavierM XavierM requested a review from a team as a code owner July 12, 2022 20:44
@elasticmachine
Copy link
Contributor

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

@XavierM XavierM requested review from Zacqary and mikecote July 12, 2022 20:44
@XavierM XavierM marked this pull request as draft July 13, 2022 00:32
@XavierM XavierM changed the title [RAM] Refactor to avoid multiple call on the rule's SO and avoid some kind of OCC in the task runner [RAM] Remove isSnoozeUntil and calculate on the fly for get/find Jul 18, 2022
@XavierM XavierM requested review from jcger and JiaweiWu and removed request for Zacqary July 26, 2022 10:01
return (
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.ruleStateFilter.disabledOptionText"
defaultMessage="Rule is disabled"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick save id and defaultMessage in a variable, set the value depending on each case and return the component once to avoid repeating code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but I can NOT because https://github.com/elastic/kibana/issues/81862

@jcger
Copy link
Contributor

jcger commented Jul 26, 2022

LGTM! wasn't able to break it locally :P

@mdefazio mdefazio self-requested a review July 26, 2022 13:33
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!

@XavierM XavierM enabled auto-merge (squash) July 26, 2022 15:48
Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Let's take a look at the copy in another PR. I think it would also be good take another look at this filter and perhaps break it apart into two separate options.

return (
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.ruleStateFilter.enabledOptionText"
defaultMessage="Rule is enabled"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this can be shortened:

Suggested change
defaultMessage="Rule is enabled"
defaultMessage="Enabled"

return (
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.ruleStateFilter.disabledOptionText"
defaultMessage="Rule is disabled"
Copy link
Contributor

@lcawl lcawl Jul 26, 2022

Choose a reason for hiding this comment

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

Ditto re shorter is better, though we'll need to revisit this term in the future:

Suggested change
defaultMessage="Rule is disabled"
defaultMessage="Disabled"

return (
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.ruleStateFilter.snoozedOptionText"
defaultMessage="Rule has snoozed"
Copy link
Contributor

@lcawl lcawl Jul 26, 2022

Choose a reason for hiding this comment

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

Though there seems to be complexity around whether the snooze is active or not, perhaps it's sufficient to just say there's a snooze schedule. If it's also supposed to cover snoozes that happened in the past (and are not recurring), perhaps "Snooze configured"?:

Suggested change
defaultMessage="Rule has snoozed"
defaultMessage="Snooze scheduled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is also possible to just have a relative snooze like I only want to snooze this rule for two hours.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me a relative snooze is still something you schedule, so IMO that text would work for both. We can pursue further options in a subsequent PR.

@XavierM XavierM merged commit cc31f24 into elastic:main Jul 26, 2022
@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
triggersActionsUi 958.8KB 959.4KB +559.0B

Page load bundle

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

id before after diff
triggersActionsUi 90.6KB 90.6KB -17.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
alert 75 74 -1

History

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

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 26, 2022
@lcawl lcawl added the ui-copy Review of UI copy with docs team is recommended label Jul 26, 2022
@gmmorris gmmorris added the Feature:Alerting/RulesManagement Issues related to the Rules Management UX label Jul 29, 2022
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:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture ui-copy Review of UI copy with docs team is recommended v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule SO is updated twice after running and is subject to OCC issues
10 participants