Rule execution log support backfill rule run types#183898
Rule execution log support backfill rule run types#183898nkhristinin merged 34 commits intoelastic:mainfrom
Conversation
|
/ci |
|
/ci |
|
/ci |
|
/ci |
|
@elasticmachine merge upstream |
e40pud
left a comment
There was a problem hiding this comment.
LGTM! Also checked locally and I can see manual rule runs. Left a nit comment.
| /> | ||
| ), | ||
| field: 'backfill', | ||
| render: (backfill: { to: string; from: string }) => { |
There was a problem hiding this comment.
Seems like FoemattedDate handles this, but anyway according to schema defined above, both to and from are optional and can be undefined.
There was a problem hiding this comment.
I upated schema, I keep backfill optional, but if it's present, then from and to also present
xcrzx
left a comment
There was a problem hiding this comment.
I tested the PR locally and found no major issues. There is a local timezone issue that is not straightforward and might need some clarification, but other than that, it looks good to me. Thank you for the PR, @nkhristinin!
.../lib/detection_engine/rule_monitoring/logic/rule_execution_log/event_log/event_log_reader.ts
Outdated
Show resolved
Hide resolved
.../security_solution/common/api/detection_engine/rule_monitoring/model/execution_result.gen.ts
Outdated
Show resolved
Hide resolved
...rity_solution/common/api/detection_engine/rule_monitoring/model/execution_result.schema.yaml
Outdated
Show resolved
Hide resolved
| if (runTypeFilters.length > 0 && runTypeFilters.length < 2) { | ||
| const ruleRunEventActions = runTypeFilters.map((runType) => { | ||
| if (runType === RuleRunTypeEnum.standard) { | ||
| return 'execute'; |
There was a problem hiding this comment.
Can we use EVENT_LOG_ACTIONS.execute here instead of the hardcoded string?
| if (runType === RuleRunTypeEnum.standard) { | ||
| return 'execute'; | ||
| } else { | ||
| return 'execute-backfill'; |
There was a problem hiding this comment.
Same here. Can we use EVENT_LOG_ACTIONS.executeBackfill here instead of the hardcoded string?
| page, | ||
| perPage, | ||
| sort, | ||
| runTypeFilters, |
There was a problem hiding this comment.
runTypeFilters is unused, please remove it from the interface
...tion_engine/rule_details_ui/pages/rule_details/execution_log_table/execution_log_columns.tsx
Show resolved
Hide resolved
| export const COLUMN_SOURCE_EVENT_TIME_RANGE_TOOLTIP = i18n.translate( | ||
| 'xpack.securitySolution.detectionEngine.ruleDetails.ruleExecutionLog.sourceEventTimeRangeTooltip', | ||
| { | ||
| defaultMessage: "Only for manual rule runs. Don't inlcude additional lookback time.", |
There was a problem hiding this comment.
Not sure I understand what "Don't inlcude additional lookback time." means. Does it refer to the displayed interval not including lookback time, or does it mean the rule was executed without lookback?
There was a problem hiding this comment.
Also, there's a typo in inlcude -> include
There was a problem hiding this comment.
Will check with docs, thanks!
| { | ||
| name: i18n.COLUMN_TYPE, | ||
| field: 'type', | ||
| sortable: false, | ||
| width: '10%', | ||
| render: (value, record) => { | ||
| return ( | ||
| <EuiText size="s"> | ||
| {record.backfill ? RULE_EXECUTION_TYPE_BACKFILL : RULE_EXECUTION_TYPE_STANDARD} | ||
| </EuiText> | ||
| ); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Should we hide this column behind the feature flag?
There was a problem hiding this comment.
Probably it's fine, as it will be scheduled rule type, which is true
There was a problem hiding this comment.
Do we need to show our users a column that only has a single value, 'Scheduled', for all rows? It would probably be best to hide it to avoid confusion for those who haven't enabled the feature flag.
| if (showSourceEventTimeRange) { | ||
| columns.push(...getSourceEventTimeRangeColumns()); | ||
| messageColumnWidth = 30; | ||
| } |
There was a problem hiding this comment.
We need to check that the feature flag is enabled here as well.
There was a problem hiding this comment.
showSourceEventTimeRange - this state controlled by switch which is hidden by feature flag
There was a problem hiding this comment.
We still need to cover the situation when the feature flag gets enabled and then disabled for any reason. When I disabled the feature flag locally, the table continued showing this column.
|
@nkhristinin Have you aligned with the docs team regarding the changes in this PR? Do we want to document them? |
I plan to have additional Ticket for this and next PR, to get feedback for UI texts and docs creation. I want to merge this PR before that, as it needed for all next PR related to this feature |
|
@elasticmachine merge upstream |
xcrzx
left a comment
There was a problem hiding this comment.
LGTM! Thank you, Nikita, for addressing my comment 👍
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
|
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Rule execution log support backfill rule run types
Screen.Recording.2024-05-22.at.13.43.13.mov
Feature flag
manualRuleRunEnabledDescription
run_type_filtersfilters as parameter with values like "standard" and "backfill"backfillHow to test
1 . Enable feature flag -
manualRuleRunEnabled2. For you rule call schedule api
/internal/alerting/rules/backfill/_schedulePOSTWith this body (put your values for rule id and date range):