-
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
Additional cleanup and improvements noticed when working on #864 #871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t of the events'. I think it's clearer than 'at the top level'.
This was added mistakenly, a long time ago, in a commit far far away.
It's meant to have a ready-made name attribute for URLs
5 tasks
@andrewkroh or @adriansr I would like your thoughts on the |
I added a few changelog entries for the changelog-worthy changes. |
ebeahan
previously approved these changes
Jun 18, 2020
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 👍
ebeahan
approved these changes
Jun 26, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to minimize the amount of noise on #864 (already a big rewrite) many small improvements have been held off. This PR introduces these pending improvements. If anything in there is contentious, I'm happy to split them off in their own PR.
Here are the changes:
object_type=keyword
for allobject
fields in ECS. This is no longer the case. A bit more on this below.@
with-
indashed_name
. This attribute is meant to be a URL-safe attribute that more or less represents the field name.nestings
attribute in favour ofreused_here
. The old representation could not capture additional information about a given reuse (e.g. specifying what field set is being reused, which is important when reusing as another name).More on
object_type
The effect of setting
object_type=keyword
on object fields is that Beats will constrain subkeys of this object field to a certain type, via a mix of dynamic template properties. However there's no reason to introduce this as a default on all object fields in ECS. This decision should be made on a case by case basis. Note also that this attribute is currently only supported by the Beats template generator, the ECS template generator currently does nothing with it.Object fields that are no longer constrained by this property added as a default are:
dns.answers
log.syslog
network.inner
observer.egress
observer.ingress
None of these object fields were meant to have dynamic templates and dynamically defined fields under them. So this should not be an issue.
The only place
object_type
makes sense in ECS is for the variouslabels
object fields. They are expected to be constrained to onlykeyword
key-value pairs. These fields are currentlylabels
andcontainer.labels
(whereobject_type=keyword
) is already defined explicitly.