Skip to content
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

add ECS support #85

Merged
merged 6 commits into from
Jun 3, 2021
Merged

Conversation

kaisecheng
Copy link
Contributor

This PR add ECS support to the filter.
It adds a warning if target is not set in ECS mode

Fixed #80

==== Event Metadata and the Elastic Common Schema (ECS)
When ECS compatibility is disabled, the value is stored in the root level.
When ECS is enabled, the value is stored in the `target` field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added the table to compare the columns of ECS disabled and ECS v1 because the field name is depending on the value of target which has no default value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the intention but from a plugin user perspective the docs seems confusing.
Maybe we should just include the same message as the one that gets logged to explain?

The plugin behaves the same regardless of ECS compatibility, except issuing a warning when `target` isn't set.
It is recommended to set the `target` option to avoid potential schema conflicts.

(just a draft - probably need rewording if we decide to use it)

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for generic plugins such as this one it would be nice to wait for logstash-plugins/logstash-mixin-ecs_compatibility_support#6 (just in case we need to change the message or do more target checks)

while at it we might also want to add field reference validation for the target

==== Event Metadata and the Elastic Common Schema (ECS)
When ECS compatibility is disabled, the value is stored in the root level.
When ECS is enabled, the value is stored in the `target` field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the intention but from a plugin user perspective the docs seems confusing.
Maybe we should just include the same message as the one that gets logged to explain?

The plugin behaves the same regardless of ECS compatibility, except issuing a warning when `target` isn't set.
It is recommended to set the `target` option to avoid potential schema conflicts.

(just a draft - probably need rewording if we decide to use it)

@kaisecheng kaisecheng requested a review from karenzone May 26, 2021 16:23
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments in line for your consideration. I think it's important to add the "...regardless of ECS compatibility" wording to make the meaning more clear. You can decide if you like the tip formatting or not. :-)

Comment on lines 29 to 30
The plugin behaves the same except giving a warning when ECS is enabled and `target` isn't set.
It is recommended to set the `target` option to avoid potential schema conflicts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The plugin behaves the same except giving a warning when ECS is enabled and `target` isn't set.
It is recommended to set the `target` option to avoid potential schema conflicts.
The plugin behaves the same regardless of ECS compatibility, except giving a warning when ECS is enabled and `target` isn't set.
TIP: Set the `target` option to avoid potential schema conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the way y'all handled this. You can use a TIP to put more emphasis on the impact of ECS. If you want to try this, it will render like this:

Screen Shot 2021-05-28 at 8 01 55 PM

@kaisecheng kaisecheng requested a review from karenzone June 1, 2021 16:21
@kaisecheng kaisecheng merged commit 4efc215 into logstash-plugins:master Jun 3, 2021
@kaisecheng kaisecheng removed the request for review from karenzone June 3, 2021 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ECS-Compatibility Mode
4 participants