[RnA] Simplify matcher context and Add Rule name to workflow payload#271768
[RnA] Simplify matcher context and Add Rule name to workflow payload#271768miguelmartin-elastic wants to merge 14 commits into
Conversation
…: description, enabled, createdAt, updatedAt
kdelemme
left a comment
There was a problem hiding this comment.
i think we can trim the Rule type too, and a few questions on where the rule map should be crafted and stored on? Might be pros/cons in both approach. what do you think?
| export interface Rule { | ||
| id: RuleId; | ||
| spaceId: string; | ||
| kind: 'alert' | 'signal'; | ||
| name: string; | ||
| description: string; | ||
| tags: string[]; | ||
| enabled: boolean; | ||
| createdAt: string; | ||
| updatedAt: string; | ||
| } |
There was a problem hiding this comment.
Can we simplify these as well? We probably don't need most of these fields?
| } | ||
|
|
||
| export interface ActionPolicyWorkflowPayloadRule { | ||
| name: string; |
There was a problem hiding this comment.
Should we add the tags as well?
There was a problem hiding this comment.
Happy to add it, but leaning towards YAGNI for now — tags are already available in the matcher context for filtering. Open to adding if you see a concrete use case here 🤔
There was a problem hiding this comment.
Tags could be useful for a message but let's wait i agree with you
| export interface Rule { | ||
| id: RuleId; | ||
| spaceId: string; | ||
| kind: 'alert' | 'signal'; |
There was a problem hiding this comment.
I think we only handle 'alert' kind so might not be useful to keep kind
| policyId: ActionPolicyId; | ||
| groupKey: Record<string, unknown>; | ||
| episodes: AlertEpisode[]; | ||
| rules: Record<RuleId, ActionPolicyWorkflowPayloadRule>; |
There was a problem hiding this comment.
If we trim Rule, can't we use Record<RuleId, Rule> directly?
There was a problem hiding this comment.
Rule still has some other fields that we don't use in the payload: id, spaceId, tags. What I did instead was to make ActionPolicyWorkflowPayloadRule be a Pick type from Rule in d75f460 Lmkwyt
| } | ||
|
|
||
| private async dispatchWorkflow( | ||
| group: ActionGroup, |
There was a problem hiding this comment.
What if we make rules (the map one) directly part of the action group?
…m/miguelmartin-elastic/kibana into feat/simplify-matcher-context-517
kdelemme
left a comment
There was a problem hiding this comment.
I think we should not rebuild the whole rules map for every episode, build it incrementally
| export interface MatcherContextRule { | ||
| id: string; | ||
| name: string; | ||
| description: string; | ||
| tags: string[]; | ||
| enabled: boolean; | ||
| createdAt: string; | ||
| updatedAt: string; | ||
| } |
| { | ||
| path: 'rules', | ||
| detail: 'Record<string, { name: string }>', | ||
| documentation: | ||
| 'Rule metadata keyed by rule id. Covers all rules present in `episodes`. Access via `rules[episode.rule_id].name`.', | ||
| }, |
There was a problem hiding this comment.
We are going to have some conflict when #271770 land :D
| const group = groupMap.get(actionGroupId)!; | ||
| group.episodes.push(episode); | ||
| group.rules = buildRulesMap([...new Set(group.episodes.map((e) => e.rule_id))], rules); |
There was a problem hiding this comment.
Are we rebuilding the rules map every time?
I think we can optimize this (you can drop your helper buildRulesMap too)
const group = groupMap.get(actionGroupId)!;
group.episodes.push(episode);
const rule = rules?.get(episode.rule_id);
if (rule) {
group.rules[episode.rule_id] = { name: rule.name };
}
💚 Build Succeeded
Metrics [docs]Async chunks
History
|

Summary
Closes https://github.com/elastic/rna-program/issues/517
Closes https://github.com/elastic/rna-program/issues/499
#517 — Simplify
MatcherContextrule fieldsRemoves
description,enabled,createdAt, andupdatedAtfromMatcherContextRuleandMATCHER_CONTEXT_FIELDS. These fields have no use in matcher evaluation logic and add unnecessary noise.#499 — Add rule metadata to dispatched workflow payload
Adds a
rules: Record<ruleId, { name: string }>map toActionPolicyWorkflowPayload. Workflows can now access rule names via{{ inputs.rules[episode.rule_id].name }}without extra lookups. Data is sourced from the existingstate.rulesalready loaded byFetchRulesStep.Checklist