-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Event Log] Extend ECS event schema with fields needed for Detection Engine #95067
[Event Log] Extend ECS event schema with fields needed for Detection Engine #95067
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me.
I'm not sure we actually have any tests that test all the fields, but I think at this point, we should add one. We end up testing the alert-specific fields in other alerting tests, so we'll know if something went bad with specific fields (hopefully). But, in general, I don't think we actually test that we can write and then read all the fields back. Seems like it's time to do that - we should have done it before I think. That way we'll know if we end up screwing up some of the fields during similar "updates".
I don't think there's any value in doing this as a unit test, we might as well get an e2e function test for this, that way we'll know everything including mappings is working the way we want. Those tests are in here: x-pack/test/plugin_api_integration
. I think we'd just add a test to write an event with all the existing fields set to some value, and then read it back, make sure all the fields are the same (hopefully just an 'equal` test with the two objects).
The README.md
should also be updated - it's currently our only doc for event log.
Presumably at some point there will be some additional provider
and action
values, so those would need to be doc'd as well - or it's not clear if we would enhance the way we write the events from alerting to somehow add the new fields for SIEM alerts.
Note that it is kinda weird to have this alerting stuff in the event log doc, but seemed like the best place to put it at the time. At some point, alerting-specific stuff should probably point into the alerting doc instead. Then same with any SIEM-specific stuff ...
Ok cool, thanks for your review @pmuellr 👍 Adding a smoke test as you described makes sense to me. I'll add one. I can also update the
Not sure I fully got this part. When we create a new provider and new action names for the detection rule execution log, they will likely be defined and documented on the Detections side.
Imho event log should not dictate and document which provider and action names are used by its clients. Personally, to me it's a bit confusing to see any client-specific things in the event_log plugin (including the |
ad3832c
to
86bd8d2
Compare
bbf99ec
to
c0176cf
Compare
The comments re: the readme being alerting specific, and describing ECS fields are appropriate, but the reason we documented them here is to make it easier for customers using the event log to diagnose alerting problems is to have a single place to look at for all the info, for right now. Otherwise everything would be spread across alerting, event log and ECS with no single place describing everything. We'll need to split it eventually ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a bunch Georgii!!
|
||
```js | ||
{ | ||
// Base ECS fields. | ||
// https://www.elastic.co/guide/en/ecs/current/ecs-base.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thx for the links!
server_uuid: "UUID of kibana server, for diagnosing multi-Kibana scenarios", | ||
alerting: { | ||
instance_id: "alert instance id, for relevant documents", | ||
action_group_id: "alert action group, for relevant documents", | ||
action_subgroup_id: "alert action subgroup, for relevant documents", | ||
action_subgroup: "alert action subgroup, for relevant documents", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
@@ -363,3 +411,14 @@ yarn test:jest x-pack/plugins/event_log --watch | |||
|
|||
See: [`x-pack/test/plugin_api_integration/test_suites/event_log`](https://github.com/elastic/kibana/tree/master/x-pack/test/plugin_api_integration/test_suites/event_log). | |||
|
|||
To develop integration tests, first start the test server from the root of the repo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
// https://www.elastic.co/guide/en/ecs/current/ecs-rule.html | ||
rule: { | ||
author: ["Elastic"], | ||
id: "a823fd56-5467-4727-acb1-66809737d943", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminded me of recent id
vs rule_id
conversations -- there's not a dedicated spot for us to put rule_id
(the static id of a rule between clusters), but we can just continue to reference it in the message
ala:
[+] Starting Signal Rule execution name: "Persistence via TelemetryController Scheduled Task Hijack" id: "6c25a284-808a-11eb-87f7-018e452bbfc9" rule id: "68921d85-d0dc-48b3-865f-43291ca2c4f2" signals index: ".siem-signals-spong-default"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spong Thank you for pointing to this problem 👍
ECS has a standard rule.uuid
field, I'm not sure about its meaning though, the docs are not super clear to me. But even if neither rule.id
nor rule.uuid
are suitable for storing a global static rule id, we could do it with a custom field like kibana.detection_engine.rule.guid
. And then submit a PR to ECS with rule.guid
field.
I'll need to figure this out - which fields to use for rule ids :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docs, e2e tests, and new fields look good too -- you've got it all! Thank you @banderror! 🙂 LGTM! 👍
c0176cf
to
a01b8fb
Compare
a01b8fb
to
8eb6bae
Compare
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @banderror |
…Engine (elastic#95067) **Related to:** elastic#94143 ## Summary This PR adds new fields to the schema (`EventSchema`, `IEvent`): - standard ECS fields: `error.*`, `event.*`, `log.level`, `log.logger`, `rule.*` - custom field set `kibana.detection_engine` We need these fields on the Detections side to implement detection rule execution log. See the related proposal (elastic#94143) for more details. Also, this PR bumps ECS used in Event Log from `1.6.0` to the current `1.8.0` version. They are 100% same in terms of fields used in Event Log, so no changes in the schema were caused by this version increment.
…Engine (#95067) (#95654) **Related to:** #94143 ## Summary This PR adds new fields to the schema (`EventSchema`, `IEvent`): - standard ECS fields: `error.*`, `event.*`, `log.level`, `log.logger`, `rule.*` - custom field set `kibana.detection_engine` We need these fields on the Detections side to implement detection rule execution log. See the related proposal (#94143) for more details. Also, this PR bumps ECS used in Event Log from `1.6.0` to the current `1.8.0` version. They are 100% same in terms of fields used in Event Log, so no changes in the schema were caused by this version increment. Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
Related to: #94143
Summary
This PR adds new fields to the schema (
EventSchema
,IEvent
):error.*
,event.*
,log.level
,log.logger
,rule.*
kibana.detection_engine
We need these fields on the Detections side to implement detection rule execution log. See the related proposal (#94143) for more details.
Also, this PR bumps ECS used in Event Log from
1.6.0
to the current1.8.0
version. They are 100% same in terms of fields used in Event Log, so no changes in the schema were caused by this version increment.Checklist