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

Not selecting any days in alerts filter maps to all weekdays #156913

Merged
merged 5 commits into from
May 9, 2023

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented May 5, 2023

Fixes: #156878

As discussed in the issue just turning if alert is generated within timeframe on should not change the filter scope.
And the UI should not confuse the users by setting some days as default.

And as discussed in #154680, default hours filter should cover the whole day.

Therefore, this PR sets default alerts filter options as:

  {
    days: [],
    hours: {
      start: '00:00',
      end: '00:00',
    },
  };

empty days array maps to all weekdays [1,2,3,4,5,6,7]
and hours 00:00 -> 00:00 maps to 00:00 -> 24:00

@ersin-erdal ersin-erdal added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0 v8.9.0 labels May 5, 2023
@ersin-erdal ersin-erdal self-assigned this May 5, 2023
@ersin-erdal ersin-erdal marked this pull request as ready for review May 6, 2023 11:52
@ersin-erdal ersin-erdal requested review from a team as code owners May 6, 2023 11:52
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

LGTM, small export nit

import { AlertHit, SummarizedAlerts } from '@kbn/alerting-plugin/server/types';
import { ParsedTechnicalFields } from '../../common';
import { ParsedExperimentalFields } from '../../common/parse_experimental_fields';
import { IRuleDataClient, IRuleDataReader } from '../rule_data_client';

const MAX_ALERT_DOCS_TO_RETURN = 100;
export const ISO_WEEKDAYS: IsoWeekday[] = [1, 2, 3, 4, 5, 6, 7];
Copy link
Contributor

Choose a reason for hiding this comment

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

Already exported from @kbn/triggers-actions-ui-plugin/common/constants, but probably should be exported from alerting-plugin. Can you move this there?

Copy link
Contributor

@benakansara benakansara left a comment

Choose a reason for hiding this comment

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

AO changes LGTM

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution / Detections : Page Filters "after each" hook for "Changed banner should hide on Reset"
  • [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution / Detections : Page Filters Changed banner should hide on Reset
  • [job] [logs] Security Solution Tests #3 / timeline flyout button the (+) button popover menu owns focus

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 105 106 +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 593 594 +1

Async chunks

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

id before after diff
alerting 183.4KB 183.3KB -21.0B
triggersActionsUi 1.4MB 1.4MB -38.0B
total -59.0B

Page load bundle

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

id before after diff
alerting 49.1KB 49.2KB +65.0B
triggersActionsUi 87.0KB 86.9KB -23.0B
total +42.0B
Unknown metric groups

API count

id before after diff
alerting 614 615 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 399 403 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 479 483 +4
total +6

History

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

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit d157389 into elastic:main May 9, 2023
@ersin-erdal ersin-erdal deleted the 156878-alerts-filter-weekdays branch May 9, 2023 15:29
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.8 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 156913

Questions ?

Please refer to the Backport tool documentation

@ersin-erdal
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

ersin-erdal added a commit to ersin-erdal/kibana that referenced this pull request May 9, 2023
…#156913)

Fixes: elastic#156878

As discussed in the issue just turning `if alert is generated within
timeframe` on should not change the filter scope.
And the UI should not confuse the users by setting some days as default.

And as discussed in elastic#154680, default hours filter should cover the whole
day.

Therefore, this PR sets default alerts filter options as:
```
  {
    days: [],
    hours: {
      start: '00:00',
      end: '00:00',
    },
  };
```

empty days array maps to all weekdays  `[1,2,3,4,5,6,7]`
and hours `00:00 -> 00:00` maps to `00:00 -> 24:00`

(cherry picked from commit d157389)

# Conflicts:
#	x-pack/plugins/rule_registry/server/utils/create_get_summarized_alerts_fn.ts
ersin-erdal added a commit that referenced this pull request May 15, 2023
…156913) (#157211)

# Backport

This will backport the following commits from `main` to `8.8`:
- [Not selecting any days in alerts filter maps to all weekdays
(#156913)](#156913)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ersin
Erdal","email":"92688503+ersin-erdal@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-09T15:29:14Z","message":"Not
selecting any days in alerts filter maps to all weekdays
(#156913)\n\nFixes: #156878\r\n\r\nAs discussed in the issue just
turning `if alert is generated within\r\ntimeframe` on should not change
the filter scope.\r\nAnd the UI should not confuse the users by setting
some days as default.\r\n\r\nAnd as discussed in #154680, default hours
filter should cover the whole\r\nday.\r\n\r\nTherefore, this PR sets
default alerts filter options as:\r\n```\r\n {\r\n days: [],\r\n hours:
{\r\n start: '00:00',\r\n end: '00:00',\r\n },\r\n
};\r\n```\r\n\r\nempty days array maps to all weekdays
`[1,2,3,4,5,6,7]`\r\nand hours `00:00 -> 00:00` maps to `00:00 ->
24:00`","sha":"d15738989a6544c9b939cde5afc3533c9850d7d4","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:ResponseOps","v8.8.0","v8.9.0"],"number":156913,"url":"https://github.com/elastic/kibana/pull/156913","mergeCommit":{"message":"Not
selecting any days in alerts filter maps to all weekdays
(#156913)\n\nFixes: #156878\r\n\r\nAs discussed in the issue just
turning `if alert is generated within\r\ntimeframe` on should not change
the filter scope.\r\nAnd the UI should not confuse the users by setting
some days as default.\r\n\r\nAnd as discussed in #154680, default hours
filter should cover the whole\r\nday.\r\n\r\nTherefore, this PR sets
default alerts filter options as:\r\n```\r\n {\r\n days: [],\r\n hours:
{\r\n start: '00:00',\r\n end: '00:00',\r\n },\r\n
};\r\n```\r\n\r\nempty days array maps to all weekdays
`[1,2,3,4,5,6,7]`\r\nand hours `00:00 -> 00:00` maps to `00:00 ->
24:00`","sha":"d15738989a6544c9b939cde5afc3533c9850d7d4"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156913","number":156913,"mergeCommit":{"message":"Not
selecting any days in alerts filter maps to all weekdays
(#156913)\n\nFixes: #156878\r\n\r\nAs discussed in the issue just
turning `if alert is generated within\r\ntimeframe` on should not change
the filter scope.\r\nAnd the UI should not confuse the users by setting
some days as default.\r\n\r\nAnd as discussed in #154680, default hours
filter should cover the whole\r\nday.\r\n\r\nTherefore, this PR sets
default alerts filter options as:\r\n```\r\n {\r\n days: [],\r\n hours:
{\r\n start: '00:00',\r\n end: '00:00',\r\n },\r\n
};\r\n```\r\n\r\nempty days array maps to all weekdays
`[1,2,3,4,5,6,7]`\r\nand hours `00:00 -> 00:00` maps to `00:00 ->
24:00`","sha":"d15738989a6544c9b939cde5afc3533c9850d7d4"}}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0 v8.9.0
Projects
None yet
6 participants