-
Notifications
You must be signed in to change notification settings - Fork 431
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
Adding support for enabled in templates and beats #824
Adding support for enabled in templates and beats #824
Conversation
@@ -1122,6 +1122,7 @@ | |||
norms: false | |||
default_field: false | |||
description: The stack trace of this error in plain text. | |||
index: false |
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.
I wonder what the final template will look like here? Will it still index the text
part but skip keyword? Or does it apply to both?
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.
Ah good catch. So I tried testing a similar scenario in ES where the outer field error.stack_trace
has index: false
but the inner one error.stack_trace.text
does not have that. Looks like error.stack_trace
is not searchable as expected, but error.stack_trace.text
will be. The template that ecs generates for this schema is here: https://github.com/elastic/ecs/blob/master/generated/elasticsearch/7/template.json#L650-L661 which hasn't changed from the edits I've made here.
@webmat is it intentional to have the error.stack_trace.text
be searchable while the error.stack_trace.keyword
is not searchable?
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.
Not intentional, no.
Also, this highlights the fact that despite flagging a few fields as index: False
in ECS, this never made it to the Beats mapping. Oops 🙊. Changing this now would cause an expected breaking change for Beats, IMO.
The addition of this feature makes total sense, but we'll need to decide what we do with this Beats change.
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.
@webmat who should I reach out to get approval for making the breaking changes for beats? On the endpoint side having ecs add the index
and enabled
fields in beats output would be very helpful because we use the beats output to build our mapping.
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.
I think @andrewkroh would be your best bet
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.
In elastic/beats log.original
and event.original
are used, but I didn't find anything that would be querying them. So I think the only effect would be if users had custom queries (like a regexp query on the event.original
).
And nothing in beats populates error.stack_trace
from what I could find. @simitt would anything in APM be affected by setting index: false
for error.stack_trace
(like does anything query error.stack_trace)? I imagine anything that needs to search it would use error.stack_trace.text
.
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.
Thanks for the ping; APM does not use event.original
, log.original
nor error.stack_trace
at the moment.
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.
Thanks for taking a look @andrewkroh and @simitt. Is there anything else we could do to mitigate the breaking change risk?
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.
Any other concerns with merging this PR?
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 @jonathan-buttner.
Just one nit, could you split the changelog in two, according to the comment below, please? You can merge right after.
CHANGELOG.next.md
Outdated
@@ -61,6 +61,7 @@ Thanks, you're awesome :-) --> | |||
* Add support for reusing offical fieldsets in custom schemas. #751 | |||
* Add full path names to reused fieldsets in `nestings` array in `ecs_nested.yml`. #803 | |||
* Allow shorthand notation for including all subfields in subsets. #805 | |||
* Adding support for `enabled` and `index` fields. #824 |
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.
* Adding support for `enabled` and `index` fields. #824 | |
* Add support for Elasticsearch `enabled` field parameter. #824 |
And in the "Bugfixes" section
* Field parameter `index` is now correctly populated in the Beats field definition file. #824
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.
Thanks @webmat done!
This PR adds support for the mapping
enabled
field.https://www.elastic.co/guide/en/elasticsearch/reference/current/enabled.html
If the
enabled
field is included and set tofalse
in a schema file (most likely a custom schema file) the generated template and beats output will include the field.enabled
is only valid forobject
andnested
types.The
enabled
field is also supported at the top level of a mapping but I have not implemented that here.I also added
index
support in the beats generator.As a side note, it would probably be useful to extend the subset format to allow for setting ecs core fields as
enabled = false
. Currently this change only allows for custom schema to explicitly be set toenabled = false
. A user would have to edit the core ecs files temporarily to force those fields to beenabled = false
. I think this can be tackled in a follow up PR if we think it'd be useful.Example generated beats output