Add Internal Alert/Signal ID to Endpoint Alert telemetry #126216

donaherc · 2022-02-23T00:14:12Z

Summary

This PR Adds the "signal_id" from the created Kibana signal to the raw endpoint alert telemetry documents. This field is needed to ensure that we can properly combine Alert telemetry with user actions on those alerts ("acknowledged", "closed", etc)

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

donaherc

Just some context for you @pjhampton

donaherc · 2022-02-23T00:14:50Z

...ck/plugins/security_solution/server/lib/detection_engine/signals/search_after_bulk_create.ts

+            logger,
+            eventsTelemetry,
+            enrichedEvents,
+            createdItems,


These are the response objects generated after the signals are created.

donaherc · 2022-02-23T00:15:36Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.ts

  buildRuleMessage: BuildRuleMessage
 ) {
  if (eventsTelemetry === undefined) {
    return;
  }
+  // Create map of ancenstor_id -> alert_id
+  let signalIdMap = new Map<string, string>();


We build a map of <endpoint_alert_id>, <signal_id> so that we can look it up when we send the payload

This code isn't pragmatic.

createdEvents.map(function (obj: SignalSource) { signalIdMap = signalIdMap.set(String(obj['kibana.alert.original_event.id']), String(obj._id)); return null; });

Yields an array of nulls where the length is the number of events. You are also side-effecting unnecessarily and not checking if the ancestor_id exists. A safer and more functional way of writing this code would be:

type AncestorId = string; type AlertId = string; const signalIdMap = createdEvents.reduce((signalMap, obj) => { const ancestorId = String(obj['kibana.alert.original_event.id']); const alertId = String(obj._id); if (ancestorId !== null && ancestorId !== undefined) { signalMap = signalIdMap.set(ancestorId, alertId); } return signalMap; }, new Map<AncestorId, AlertId>())

This results in 1 array instead of 2. I think I would also like to see a unit test around this function - one where the ancestor id exists and another when it doesn't.

x-pack/plugins/security_solution/server/lib/telemetry/filters.ts

elasticmachine · 2022-02-23T00:20:32Z

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

donaherc · 2022-02-23T00:21:06Z

@elasticmachine merge upstream

donaherc · 2022-02-23T01:55:17Z

@elasticmachine merge upstream

pjhampton

I think the logic is there - but it needs polish. Thanks for the hard work you put into this!

I won't be approving this for 8.1 as it wouldn't be professional to merge this a couple of hours before the last release BC. With the way Elastic ships stack components there are no take backs. I want to observe this code running in staging first and analyze if we have an uptick in telemetry burned - you have made changes to security telemetry hot path. We can merge this next week and backport it into the 8.1 branch if you want this to go out in 8.1.1. You are welcome to get a second opinion of course.

pjhampton · 2022-02-23T10:21:30Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.ts

  buildRuleMessage: BuildRuleMessage
 ) {
  if (eventsTelemetry === undefined) {
    return;
  }
+  // Create map of ancenstor_id -> alert_id
+  let signalIdMap = new Map<string, string>();


This code isn't pragmatic.

createdEvents.map(function (obj: SignalSource) { signalIdMap = signalIdMap.set(String(obj['kibana.alert.original_event.id']), String(obj._id)); return null; });

Yields an array of nulls where the length is the number of events. You are also side-effecting unnecessarily and not checking if the ancestor_id exists. A safer and more functional way of writing this code would be:

type AncestorId = string; type AlertId = string; const signalIdMap = createdEvents.reduce((signalMap, obj) => { const ancestorId = String(obj['kibana.alert.original_event.id']); const alertId = String(obj._id); if (ancestorId !== null && ancestorId !== undefined) { signalMap = signalIdMap.set(ancestorId, alertId); } return signalMap; }, new Map<AncestorId, AlertId>())

This results in 1 array instead of 2. I think I would also like to see a unit test around this function - one where the ancestor id exists and another when it doesn't.

pjhampton · 2022-02-23T10:27:55Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.ts

  // @ts-expect-error @elastic/elasticsearch _source is optional
  const sources: TelemetryEvent[] = filteredEvents.hits.hits.map(function (
    obj: SearchResultWithSource
  ): TelemetryEvent {
+    obj._source.signal_id = signalIdMap.get(obj._source.event.id);


You could be doing a lot of unnecessary work here, particularly if the security user isn't using the endpoint package, and using other detection rules. I'm also not sure how I feel about this mutation, particularly because .event.id isn't necessarily read-safe.

It might be intuitive to write this as:

-> Collect Telemetry Events -> Filter for Endpoint Alerts -> Enrich with ancestor signal id, default null -> return

Yeah that flow sounds better, I can do that.

I think "ancestor signal id" is actually confusing/ maybe not semantically correct either since in signal parlance, a detection engine alert can have multiple ancestors (It's an array in the return). Do you think the name signal_id is sufficient or would you prefer another name?

.../plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.test.ts

pjhampton

🌔 🚀 ✨ LGTM ✨ 🚀 🌔

donaherc · 2022-03-01T23:29:04Z

@elasticmachine merge upstream

donaherc · 2022-03-02T00:52:08Z

Oof @pjhampton my logic to not enrich or queue events if none were created I think broke this test?
https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/query/create_query_alert_type.test.ts#L110

The doc in this test doesn't remotely resemble an endpoint alert, and was just executing eventsTelemetry.queueTelemetryEvents on an empty list I think.

donaherc · 2022-03-02T01:09:32Z

@elasticmachine merge upstream

donaherc · 2022-03-02T02:32:53Z

@elasticmachine merge upstream

donaherc · 2022-03-02T07:24:43Z

Rerunning because it's flaky.

donaherc · 2022-03-02T07:24:55Z

@elasticmachine merge upstream

x-pack/plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.ts

pjhampton · 2022-03-02T09:36:27Z

@elasticmachine merge upstream

donaherc · 2022-03-02T17:29:34Z

@elasticmachine merge upstream

…ghts

…ince it satisfies the key:string

…hment

…alerts

…e by instatiating map prior to reduce

donaherc · 2022-03-02T22:49:11Z

@elasticmachine merge upstream

kibana-ci · 2022-03-03T00:38:33Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #18 / apis Machine Learning filters get_filters should return 400 if filterId does not exist

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`securitySolution`	66	67	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	514	+1

History

💔 Build #27207 failed b223ddd
💚 Build #27056 succeeded b7209b7
💛 Build #27025 was flaky 394e050
💔 Build #27022 failed ebb9380
💔 Build #27009 failed 79e570a

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

cc @donaherc

) * Attach the internal signal_id to the endpoint alert to join with insights * Ensure we forward signal_id field properly * Don't think we need to explicitly define the field in the top-level since it satisfies the key:string * Updated unit test to check for signal id enrichment * Addressed some comments about alert_id enrichment * Refactored send_telemetry_events to be more performant and idiomatic * Added test cases with a non-matching enrichment or non-existing enrichment * Broke some tests that don't assume QueueTelemetryEvents are endpoint.alerts * my types were still off * Addressed comments to use more idiomatic 'toString' function * Fixed 'Cannot access signalIdMap before initialization name' in reduce by instatiating map prior to reduce Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 2f083bf)

kibanamachine · 2022-03-03T00:41:55Z

💚 All backports created successfully

Status	Branch	Result
✅	8.1

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

Questions ?

Please refer to the Backport tool documentation

…126739) * Attach the internal signal_id to the endpoint alert to join with insights * Ensure we forward signal_id field properly * Don't think we need to explicitly define the field in the top-level since it satisfies the key:string * Updated unit test to check for signal id enrichment * Addressed some comments about alert_id enrichment * Refactored send_telemetry_events to be more performant and idiomatic * Added test cases with a non-matching enrichment or non-existing enrichment * Broke some tests that don't assume QueueTelemetryEvents are endpoint.alerts * my types were still off * Addressed comments to use more idiomatic 'toString' function * Fixed 'Cannot access signalIdMap before initialization name' in reduce by instatiating map prior to reduce Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 2f083bf) Co-authored-by: Chris Donaher <[email protected]>

donaherc requested review from a team as code owners February 23, 2022 00:14

donaherc requested a review from pjhampton February 23, 2022 00:14

donaherc commented Feb 23, 2022

View reviewed changes

donaherc self-assigned this Feb 23, 2022

donaherc added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v8.2.0 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 23, 2022

donaherc added the Feature:Telemetry label Feb 23, 2022

donaherc force-pushed the alert-telemetry-signal-id branch from 5b68982 to 5d0e015 Compare February 23, 2022 01:54

pjhampton reviewed Feb 23, 2022

View reviewed changes

donaherc force-pushed the alert-telemetry-signal-id branch 2 times, most recently from 5e6fd29 to 3ba3958 Compare March 1, 2022 15:11

donaherc added v8.1.1 and removed v8.1.0 labels Mar 1, 2022

pjhampton approved these changes Mar 1, 2022

View reviewed changes

pjhampton reviewed Mar 2, 2022

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/signals/send_telemetry_events.ts Outdated Show resolved Hide resolved

donaherc force-pushed the alert-telemetry-signal-id branch from b814eae to ed83033 Compare March 2, 2022 19:06

donaherc added 11 commits March 2, 2022 12:06

Attach the internal signal_id to the endpoint alert to join with insi…

6ef3d2d

…ghts

Ensure we forward signal_id field properly

50d8fcb

Don't think we need to explicitly define the field in the top-level s…

123fb64

…ince it satisfies the key:string

Updated unit test to check for signal id enrichment

920e59a

Addressed some comments about alert_id enrichment

9785958

Refactored send_telemetry_events to be more performant and idiomatic

1a95e05

Added test cases with a non-matching enrichment or non-existing enric…

72c6a23

…hment

Broke some tests that don't assume QueueTelemetryEvents are endpoint.…

9c59f24

…alerts

my types were still off

717930b

Addressed comments to use more idiomatic 'toString' function

f1b6f03

Fixed 'Cannot access signalIdMap before initialization name' in reduc…

b223ddd

…e by instatiating map prior to reduce

donaherc force-pushed the alert-telemetry-signal-id branch from ed83033 to b223ddd Compare March 2, 2022 19:06

Merge branch 'main' into alert-telemetry-signal-id

d30150f

donaherc merged commit 2f083bf into main Mar 3, 2022

donaherc deleted the alert-telemetry-signal-id branch March 3, 2022 00:39

kibanamachine mentioned this pull request Mar 3, 2022

[8.1] Add Internal Alert/Signal ID to Endpoint Alert telemetry (#126216) #126739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Internal Alert/Signal ID to Endpoint Alert telemetry #126216

Add Internal Alert/Signal ID to Endpoint Alert telemetry #126216

donaherc commented Feb 23, 2022 •

edited by pjhampton

Loading

donaherc left a comment

donaherc Feb 23, 2022

donaherc Feb 23, 2022 •

edited

Loading

pjhampton Feb 23, 2022 •

edited

Loading

elasticmachine commented Feb 23, 2022

donaherc commented Feb 23, 2022

donaherc commented Feb 23, 2022

pjhampton left a comment

pjhampton Feb 23, 2022 •

edited

Loading

pjhampton Feb 23, 2022

donaherc Feb 23, 2022

donaherc Feb 23, 2022

pjhampton left a comment

donaherc commented Mar 1, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022 •

edited

Loading

donaherc commented Mar 2, 2022

pjhampton commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

kibana-ci commented Mar 3, 2022

ESLint disabled in files

Total ESLint disabled count

kibanamachine commented Mar 3, 2022

Add Internal Alert/Signal ID to Endpoint Alert telemetry #126216

Add Internal Alert/Signal ID to Endpoint Alert telemetry #126216

Conversation

donaherc commented Feb 23, 2022 • edited by pjhampton Loading

Summary

Checklist

donaherc left a comment

Choose a reason for hiding this comment

donaherc Feb 23, 2022

Choose a reason for hiding this comment

donaherc Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

pjhampton Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 23, 2022

donaherc commented Feb 23, 2022

donaherc commented Feb 23, 2022

pjhampton left a comment

Choose a reason for hiding this comment

pjhampton Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

pjhampton Feb 23, 2022

Choose a reason for hiding this comment

donaherc Feb 23, 2022

Choose a reason for hiding this comment

donaherc Feb 23, 2022

Choose a reason for hiding this comment

pjhampton left a comment

Choose a reason for hiding this comment

donaherc commented Mar 1, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022 • edited Loading

donaherc commented Mar 2, 2022

pjhampton commented Mar 2, 2022

donaherc commented Mar 2, 2022

donaherc commented Mar 2, 2022

kibana-ci commented Mar 3, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

ESLint disabled in files

Total ESLint disabled count

History

kibanamachine commented Mar 3, 2022

💚 All backports created successfully

Questions ?

donaherc commented Feb 23, 2022 •

edited by pjhampton

Loading

donaherc Feb 23, 2022 •

edited

Loading

pjhampton Feb 23, 2022 •

edited

Loading

pjhampton Feb 23, 2022 •

edited

Loading

donaherc commented Mar 2, 2022 •

edited

Loading