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

[Security Solution][Detections] Rule Execution Log Feedback and Fixes Part Deux #130072

Merged
merged 39 commits into from
May 4, 2022

Conversation

spong
Copy link
Member

@spong spong commented Apr 13, 2022

Summary

Addresses feedback and fixes identified in #126215 & #129003

Feedback addressed includes:
  • Adds toast for restoring global query state after performing view alerts for execution action

  • Fix duration hours bug (7 hours (25033167ms) as 06:417:13:000)

  • Support disabled rule platform error ([Security Solution][Detections] Adds rule execution log table #126215 (comment))
    • Updated getAggregateExecutionEvents to fallback to platform status from event.outcome if security_status is empty, and also falls back to error.message is security_message is empty. This also now queries for corresponding event.outcome if filter is provided so that platform-only events can still be displayed when filtering.


Remaining follow-ups:

None! 🎉

Checklist

Delete any items that are not applicable to this PR.

@spong spong added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Monitoring Security Solution Detection Rule Monitoring area Team:Detection Rule Management Security Detection Rule Management Team labels Apr 13, 2022
@spong spong self-assigned this Apr 13, 2022
spong and others added 18 commits April 13, 2022 08:59
…ong/kibana into rule-execution-log-feedback-part-deux
@MadameSheema
Copy link
Member

@elasticmachine merge upstream

@spong
Copy link
Member Author

spong commented May 2, 2022

A side note; not related to this PR.

We should probably clean up the error message before writing it to the event log. We concat error messages with additional info, like rule name, id, space, etc. That information makes sense when we write to the system log (console or json), but not very useful in the context of the event log and only clutters the table:

Good catch! Opened a dedicated bug/chore for this one since this PR has grown a bit already: #131352

@spong spong requested a review from a team as a code owner May 2, 2022 21:03
Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM 👍🏽

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

Checked out and tested locally, everything works as expected, code changes LGTM 👍
Thank you, @spong, for these improvements 🎉

@spong spong added the auto-backport Deprecated - use backport:version if exact versions are needed label May 3, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2787 2788 +1

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB +3.6KB

Page load bundle

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

id before after diff
securitySolution 249.2KB 249.3KB +159.0B

History

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

cc @spong

@@ -19,8 +19,9 @@ export const indexEventLogExecutionEvents = async (
log: ToolingLog,
events: object[]
): Promise<void> => {
const aliases = await es.cat.aliases({ format: 'json', name: '.kibana-event-log-*' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, why this call versus es.indices.getAlias? The formatting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! IIRC you can't JSON format the response from getAlias -- I had tried it first then swapped over to es.cat.aliases as the response was easier to parse. Not much in documentation on the client, but the endpoint API's mention it (though don't have a sample response for getAlias... 🤷).

Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @spong !

@spong spong merged commit 683463e into elastic:main May 4, 2022
@spong spong deleted the rule-execution-log-feedback-part-deux branch May 4, 2022 19:22
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.2 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 130072

Questions ?

Please refer to the Backport tool documentation

spong added a commit to spong/kibana that referenced this pull request May 4, 2022
… Part Deux (elastic#130072)

## Summary

Addresses feedback and fixes identified in elastic#126215 & elastic#129003

##### Feedback addressed includes:
* Adds toast for restoring global query state after performing `view alerts for execution` action
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511565-b77d3dc8-a8b5-4927-a947-54966a58c74f.gif" />
</p>

* Updates global SuperDatePicker to daterange of execution (+/- day) for `view alerts for execution` action (and clear all other filters)
  * See above gif
* Remove redundant `RuleExecutionStatusType` (elastic#129003 (comment))
* Persist table state (DatePicker/StatusFilter/SortField/SortOrder/Pagination) when navigating to other tabs on the same page
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164512498-59416601-d967-4a27-b0cc-0715cc0662c0.gif" />
</p>

* Fix duration hours bug (`7 hours (25033167ms)` as `06:417:13:000`)
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511478-bf0bb6d8-d8b7-4c86-8fbd-b60090f00555.png" />
</p>

* Support `disabled rule` platform error (elastic#126215 (comment))
  * Updated `getAggregateExecutionEvents` to fallback to platform status from `event.outcome` if `security_status` is empty, and also falls back to `error.message` is `security_message` is empty. This also now queries for corresponding `event.outcome` if filter is provided so that platform-only events can still be displayed when filtering.
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164510056-1e0bce86-8360-4d46-b591-2041457e3244.png" />
</p>

* Verify StatusFilter issue elastic#126215 (comment)
  * Unable to reproduce, I believe the query updates around first querying for status may've fixed this?
* Provide helpful defaults for `to`/`from` and support datemath strings again (elastic#129003 (comment))
  * Created enhancement for this here: elastic#131095
* Adds UI Unit tests for RuleExecutionLog Table
* Finalize API Integration tests for gap remediation events
  * Test methods developed for injecting arbitrary execution events while still working with event-log RBAC. See last [API integration test](https://github.com/elastic/kibana/blob/22cc0c8dbd2a1300675caf4c6d471d211ed44858/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/get_rule_execution_events.ts#L121-L166) for technique. This can further be used to inject many execution events and expand tests around pagination, sorting, filters, etc.
* Fixes `gap_duration`'s of `1-499`ms showing up as `-` instead of `0`
* Fixes restore filters action to restore either absolute or relative datepicker as it originally was
* Resolves elastic#130946
  * Adds `min-height` to tab container
  * Removes scroll-pane from ExceptionsViewer to match Alerts/Execution Log
---

##### Remaining follow-ups:

None! 🎉

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [X] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))

(cherry picked from commit 683463e)

# Conflicts:
#	x-pack/plugins/security_solution/cypress/tasks/alerts.ts
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/execution_log_table/execution_log_columns.tsx
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/event_log_reader.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/get_execution_event_aggregation/index.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/get_execution_event_aggregation/index.ts
#	x-pack/test/detection_engine_api_integration/utils/index_event_log_execution_events.ts
spong added a commit that referenced this pull request May 5, 2022
… Fixes Part Deux (#130072) (#131574)

* [Security Solution][Detections] Rule Execution Log Feedback and Fixes Part Deux (#130072)

## Summary

Addresses feedback and fixes identified in #126215 & #129003

##### Feedback addressed includes:
* Adds toast for restoring global query state after performing `view alerts for execution` action
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511565-b77d3dc8-a8b5-4927-a947-54966a58c74f.gif" />
</p>

* Updates global SuperDatePicker to daterange of execution (+/- day) for `view alerts for execution` action (and clear all other filters)
  * See above gif
* Remove redundant `RuleExecutionStatusType` (#129003 (comment))
* Persist table state (DatePicker/StatusFilter/SortField/SortOrder/Pagination) when navigating to other tabs on the same page
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164512498-59416601-d967-4a27-b0cc-0715cc0662c0.gif" />
</p>

* Fix duration hours bug (`7 hours (25033167ms)` as `06:417:13:000`)
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511478-bf0bb6d8-d8b7-4c86-8fbd-b60090f00555.png" />
</p>

* Support `disabled rule` platform error (#126215 (comment))
  * Updated `getAggregateExecutionEvents` to fallback to platform status from `event.outcome` if `security_status` is empty, and also falls back to `error.message` is `security_message` is empty. This also now queries for corresponding `event.outcome` if filter is provided so that platform-only events can still be displayed when filtering.
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164510056-1e0bce86-8360-4d46-b591-2041457e3244.png" />
</p>

* Verify StatusFilter issue #126215 (comment)
  * Unable to reproduce, I believe the query updates around first querying for status may've fixed this?
* Provide helpful defaults for `to`/`from` and support datemath strings again (#129003 (comment))
  * Created enhancement for this here: #131095
* Adds UI Unit tests for RuleExecutionLog Table
* Finalize API Integration tests for gap remediation events
  * Test methods developed for injecting arbitrary execution events while still working with event-log RBAC. See last [API integration test](https://github.com/elastic/kibana/blob/22cc0c8dbd2a1300675caf4c6d471d211ed44858/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/get_rule_execution_events.ts#L121-L166) for technique. This can further be used to inject many execution events and expand tests around pagination, sorting, filters, etc.
* Fixes `gap_duration`'s of `1-499`ms showing up as `-` instead of `0`
* Fixes restore filters action to restore either absolute or relative datepicker as it originally was
* Resolves #130946
  * Adds `min-height` to tab container
  * Removes scroll-pane from ExceptionsViewer to match Alerts/Execution Log
---

##### Remaining follow-ups:

None! 🎉

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [X] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))

(cherry picked from commit 683463e)

# Conflicts:
#	x-pack/plugins/security_solution/cypress/tasks/alerts.ts
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/execution_log_table/execution_log_columns.tsx
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/event_log_reader.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/get_execution_event_aggregation/index.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/event_log/get_execution_event_aggregation/index.ts
#	x-pack/test/detection_engine_api_integration/utils/index_event_log_execution_events.ts

* Fixing import
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
… Part Deux (elastic#130072)

## Summary

Addresses feedback and fixes identified in elastic#126215 & elastic#129003

##### Feedback addressed includes:
* Adds toast for restoring global query state after performing `view alerts for execution` action
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511565-b77d3dc8-a8b5-4927-a947-54966a58c74f.gif" />
</p>

* Updates global SuperDatePicker to daterange of execution (+/- day) for `view alerts for execution` action (and clear all other filters)
  * See above gif
* Remove redundant `RuleExecutionStatusType` (elastic#129003 (comment))
* Persist table state (DatePicker/StatusFilter/SortField/SortOrder/Pagination) when navigating to other tabs on the same page
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164512498-59416601-d967-4a27-b0cc-0715cc0662c0.gif" />
</p>

* Fix duration hours bug (`7 hours (25033167ms)` as `06:417:13:000`)
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164511478-bf0bb6d8-d8b7-4c86-8fbd-b60090f00555.png" />
</p> 

* Support `disabled rule` platform error (elastic#126215 (comment))
  * Updated `getAggregateExecutionEvents` to fallback to platform status from `event.outcome` if `security_status` is empty, and also falls back to `error.message` is `security_message` is empty. This also now queries for corresponding `event.outcome` if filter is provided so that platform-only events can still be displayed when filtering.
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/164510056-1e0bce86-8360-4d46-b591-2041457e3244.png" />
</p>

* Verify StatusFilter issue elastic#126215 (comment)
  * Unable to reproduce, I believe the query updates around first querying for status may've fixed this?
* Provide helpful defaults for `to`/`from` and support datemath strings again (elastic#129003 (comment))
  * Created enhancement for this here: elastic#131095
* Adds UI Unit tests for RuleExecutionLog Table
* Finalize API Integration tests for gap remediation events
  * Test methods developed for injecting arbitrary execution events while still working with event-log RBAC. See last [API integration test](https://github.com/elastic/kibana/blob/22cc0c8dbd2a1300675caf4c6d471d211ed44858/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/get_rule_execution_events.ts#L121-L166) for technique. This can further be used to inject many execution events and expand tests around pagination, sorting, filters, etc.
* Fixes `gap_duration`'s of `1-499`ms showing up as `-` instead of `0`
* Fixes restore filters action to restore either absolute or relative datepicker as it originally was
* Resolves elastic#130946
  * Adds `min-height` to tab container
  * Removes scroll-pane from ExceptionsViewer to match Alerts/Execution Log
---

##### Remaining follow-ups:

None! 🎉 






### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [X] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
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 Feature:Rule Monitoring Security Solution Detection Rule Monitoring area release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture v8.2.1 v8.3.0
Projects
None yet
10 participants