[Security Solution] Improve rule execution logging #166070

maximpn · 2023-09-08T11:47:47Z

Relates to: #126063

Summary

This PR extends rule event log's filter and improves log messages.

Details

We have Rule execution log feature hidden by a feature flag and disabled, it's shown on a rule details page when enabled.

The feature is close to a releasable state but some tasks had to be addressed to make it usable. This PR addresses the following tasks to make rule execution log feature releasable

Adds search bar to search for specific messages

Adds a date range filter by default set to show logs for last 24 hours

Improves error, warning and debug messages
Returns rule metrics in a message as a serialized JSON

Adds execution_id to the response

Tasks to address later

Further improve logging messages. We have either error, warning or debug messages. In fact info or trace levels aren't used but it can give useful info.
Add an OpenAPI spec for the rule execution log endpoint

elasticmachine · 2023-09-12T14:21:58Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2023-09-12T14:22:00Z

Pinging @elastic/security-solution (Team: SecuritySolution)

ashokaditya · 2023-09-13T10:03:28Z

x-pack/plugins/security_solution/public/detection_engine/rule_monitoring/api/api_client.ts

@@ -5,6 +5,7 @@
 * 2.0.
 */

+import _ from 'lodash';


I'm not sure if our bundler already imports only the used methods here but consider doing named imports for omitBy and isUndefined

Good point 👍 I relied on my IDE to add an import and automatic checks but it seems we don't control such imports automatically. Searching in the codebase shows only specific function imports. So it makes to change to importing directly omitBy and isUndefined.

ashokaditya · 2023-09-13T10:09:46Z

...lugins/security_solution/public/detection_engine/rule_monitoring/api/api_client_interface.ts

 export interface FetchRuleExecutionEventsArgs extends RuleMonitoringApiCallArgs {
  /**
   * Saved Object ID of the rule (`rule.id`, not static `rule.rule_id`).
   */
  ruleId: string;

+  /**
+   * Filter by event message. If set, result will include only events matching the search term.


nit:

Suggested change

* Filter by event message. If set, result will include only events matching the search term.

* Filter by event message. If set, results will include events matching the search term.

ashokaditya · 2023-09-13T10:10:10Z

...lugins/security_solution/public/detection_engine/rule_monitoring/api/api_client_interface.ts

@@ -62,6 +72,11 @@ export interface FetchRuleExecutionEventsArgs extends RuleMonitoringApiCallArgs
   */
  logLevels?: LogLevel[];

+  /**
+   * Filter by date range. If set, result will include only events recorded in the specified date range.


nit:

Suggested change

* Filter by date range. If set, result will include only events recorded in the specified date range.

* Filter by date range. If set, results will include events recorded in the specified date range.

...toring/components/execution_events_table/event_date_range_filter/event_date_range_filter.tsx

ashokaditya · 2023-09-13T10:19:35Z

...e_monitoring/components/execution_events_table/event_message_filter/event_message_filter.tsx

+  onChange: (value: string) => void;
+}
+
+export function EventMessageFilter({ value, onChange }: EventMessageFilterProps): JSX.Element {


Also this component. Please use React.memo

My answer the same as here. Wrapping every possible component in React.memo doesn't guarantee a performance boost and it doesn't come for free as it also leads to increased memory consumption. Ideally we should wrap only heavy components where render function performs a lot of operations and it's usually true for data grids, lists, components higher in a component tree hierarchy.

Do you have some examples of troubles with such components?

Agree with your point on using memo correctly. It does add more JS for the browser to parse and execute and we should use memo sparingly. Is there a reason you want to create a wrapper component around EuiFieldSearch instead of using it as is in execution_events_table.tsx

EventMessageFilter solves an interfacing issue. It happens because components may return change events like EuiFieldSearch's onChange accepts ChangeEvent. It's an implementation detail and should be incapsulated. This way useFilter only operates on actual values while implementation detail can rely on whatever component. handleChange inside EventMessageFilter is an adapter transforming ChangeEvent<HTMLInputElement> into a simpler text value. This way we can change the implementation e.g. replace EuiFieldSearch with something else without changing anything outside of this component. Yes, handleChange could be defined in ExecutionEventsTable but it won't conform to EventMessageFilter and LogLevelFilter and break a consisting approach. It's not a case for EuiSuperDatePicker so I made according changes by using it directly. If it required some transformation it'd be logical to wrap it in a component.

Thanks for the explanation @maximpn.

ashokaditya · 2023-09-13T10:43:52Z

x-pack/plugins/security_solution/public/detection_engine/rule_monitoring/api/api_client.ts

+    const startDate = dateRange?.start ? dateMath.parse(dateRange.start)?.toISOString() : undefined;
+    const endDate = dateRange?.end
+      ? dateMath.parse(dateRange.end, { roundUp: true })?.toISOString()
+      : undefined;


Suggested change

const startDate = dateRange?.start ? dateMath.parse(dateRange.start)?.toISOString() : undefined;

const endDate = dateRange?.end

? dateMath.parse(dateRange.end, { roundUp: true })?.toISOString()

: undefined;

const startDate = dateMath.parse(dateRange?.start ?? '')?.toISOString();

const endDate = dateMath.parse(dateRange?.end ?? '', { roundUp: true })?.toISOString();

I'm afraid dateMath.parse('') is undetermined and it will be unclear what this code does without deep diving into dateMath.parse's code or running it.

dateMath.parse('') will return undefined when an empty string is passed. So my suggestion is just a simplification of the earlier code.

My concern was an unexpected change of such behavior due to changes to dateMath. But we can mitigate it by properly testing it which I did and applying your suggestion. Thank you for helping to look under a different angle 👍 it will help to make sure the behavior is stable

Thanks for the changes.

ashokaditya · 2023-09-13T10:47:54Z

x-pack/plugins/security_solution/public/detection_engine/rule_monitoring/api/api_client.ts

+        {
+          search_term: searchTerm?.length ? searchTerm : undefined,
+          event_types: eventTypes?.length ? eventTypes.join(',') : undefined,
+          log_levels: logLevels?.length ? logLevels?.join(',') : undefined,


Suggested change

log_levels: logLevels?.length ? logLevels?.join(',') : undefined,

log_levels: logLevels?.length ? logLevels.join(',') : undefined,

dhurley14

Detections changes LGTM 👍

…ePicker

kibana-ci · 2023-09-16T00:24:25Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: fa54d80

Failed CI Steps

FTR Configs #24

Test Failures

[job] [logs] FTR Configs #24 / apis UI Counters UI Counters API stores ui counter events in usage counters savedObjects

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4505	4508	+3

Async chunks

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

id	before	after	diff
`securitySolution`	12.6MB	12.6MB	+1.8KB

History

💚 Build #159284 succeeded 0d0529ea753de774b589b7525433cf2dee75f754
💔 Build #158704 failed 01b6171d0f2f85a98e3dc025d5b7eaa5330ee684
💔 Build #158603 failed 50ccd5c230d809bbb41762ee450218442f2dc991
💔 Build #158371 failed 8f54db10b5529a4f9712d239a5c28ad625de4092
💛 Build #158005 was flaky f3c79600deeae33153cadd5a8cf51598cdf4b86b
💔 Build #157935 failed f8556710182806b106ee1465d3a1bc4d9de5c62d

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

cc @maximpn

dplumlee · 2023-09-18T19:08:25Z

...s/security_solution/server/lib/detection_engine/rule_types/utils/search_after_bulk_create.ts

      return createSearchAfterReturnType({
        success: false,
        errors: ['malformed date tuple'],
      });
    }

    while (toReturn.createdSignalsCount <= tuple.maxSignals) {
+      const cycleNum = `cycle ${searchingIteration++}`;


We might want to add documentation somewhere for what a cycle is in this instance if we're exposing these logs a lot more. At least for internal use. Especially since we could have multiple cycle 1, 2, etc... per tuple in the rule run

dplumlee

This all looks good and works with my manual testing 👍

maximpn self-assigned this Sep 8, 2023

maximpn force-pushed the improve-rule-event-log branch from 041e660 to dc87714 Compare September 8, 2023 21:26

maximpn changed the title ~~[Security Solution] Improve rule event log~~ [Security Solution] Improve rule execution logging Sep 10, 2023

maximpn force-pushed the improve-rule-event-log branch from f855671 to f3c7960 Compare September 11, 2023 14:15

maximpn marked this pull request as ready for review September 12, 2023 14:21

maximpn requested review from a team as code owners September 12, 2023 14:21

maximpn requested review from ashokaditya and parkiino September 12, 2023 14:21

maximpn requested a review from jpdjere September 12, 2023 14:21

maximpn force-pushed the improve-rule-event-log branch from f3c7960 to 8f54db1 Compare September 12, 2023 14:23

maximpn removed the request for review from jpdjere September 13, 2023 09:28

maximpn force-pushed the improve-rule-event-log branch from 8f54db1 to 50ccd5c Compare September 13, 2023 09:46

ashokaditya reviewed Sep 13, 2023

View reviewed changes

ashokaditya requested a review from dasansol92 September 13, 2023 10:51

maximpn force-pushed the improve-rule-event-log branch from 50ccd5c to 01b6171 Compare September 13, 2023 13:19

maximpn added the release_note:skip Skip the PR/issue when compiling release notes label Sep 13, 2023

dhurley14 approved these changes Sep 13, 2023

View reviewed changes

maximpn force-pushed the improve-rule-event-log branch from 01b6171 to 0d0529e Compare September 14, 2023 21:51

maximpn added 21 commits September 15, 2023 16:15

extend rule log filter with search by message filter

fc279c2

add event log date range filter

7eafb45

fix tests

967410c

return serialized metrics in a message

3817c85

add execution id to the event log response

30a9963

avoid logging the whole result output

cac2f39

update alert bulk create messages

257fbcf

update fetchRuleExecutionEvents mock

c940e1b

fix execution_event_mock

e1a6cb9

fix unit tests

0013a9d

rectify error messages

46d9c84

rectify warning log messages

d1af964

fix string interpolation in a log message

886868c

use super date picker filter rule's execution log

925d14c

rename execution to execution_id

776ce63

import only specific lodash functions

149e9d2

fix description comment

4a25f15

remove unnecessary optional chaining

bcfa8f0

simplify date range transformation

d5d9280

get rid of EventDateRangeFilter in favor of direct use of EuiSuperDat…

1796753

…ePicker

roll back unrelated changes

fa54d80

maximpn force-pushed the improve-rule-event-log branch from 236dcd4 to fa54d80 Compare September 15, 2023 23:15

ashokaditya approved these changes Sep 18, 2023

View reviewed changes

dplumlee self-requested a review September 18, 2023 14:40

dplumlee reviewed Sep 18, 2023

View reviewed changes

dplumlee approved these changes Sep 18, 2023

View reviewed changes

maximpn merged commit 6a96906 into elastic:main Sep 19, 2023

kibanamachine added the backport:skip This commit does not require backporting label Sep 19, 2023

maximpn deleted the improve-rule-event-log branch September 19, 2023 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Improve rule execution logging #166070

[Security Solution] Improve rule execution logging #166070

maximpn commented Sep 8, 2023 •

edited

Loading

elasticmachine commented Sep 12, 2023

elasticmachine commented Sep 12, 2023

ashokaditya Sep 13, 2023

maximpn Sep 13, 2023

ashokaditya Sep 13, 2023

ashokaditya Sep 13, 2023

ashokaditya Sep 13, 2023

maximpn Sep 13, 2023

ashokaditya Sep 14, 2023 •

edited

Loading

maximpn Sep 15, 2023

ashokaditya Sep 18, 2023

ashokaditya Sep 13, 2023

maximpn Sep 13, 2023

ashokaditya Sep 14, 2023

maximpn Sep 15, 2023

ashokaditya Sep 18, 2023

ashokaditya Sep 13, 2023

dhurley14 left a comment

kibana-ci commented Sep 16, 2023

dplumlee Sep 18, 2023

dplumlee left a comment

	* Filter by event message. If set, result will include only events matching the search term.
	* Filter by event message. If set, results will include events matching the search term.

	* Filter by date range. If set, result will include only events recorded in the specified date range.
	* Filter by date range. If set, results will include events recorded in the specified date range.

	log_levels: logLevels?.length ? logLevels?.join(',') : undefined,
	log_levels: logLevels?.length ? logLevels.join(',') : undefined,

[Security Solution] Improve rule execution logging #166070

[Security Solution] Improve rule execution logging #166070

Conversation

maximpn commented Sep 8, 2023 • edited Loading

Summary

Details

Tasks to address later

elasticmachine commented Sep 12, 2023

elasticmachine commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokaditya Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhurley14 left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 16, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

History

Choose a reason for hiding this comment

dplumlee left a comment

Choose a reason for hiding this comment

maximpn commented Sep 8, 2023 •

edited

Loading

ashokaditya Sep 14, 2023 •

edited

Loading