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 #36

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Jul 15, 2021

ECS support add target and [event][original]

Fixed: #34

@kaisecheng kaisecheng changed the title add ECS support [WIP] add ECS support Jul 15, 2021
@kaisecheng kaisecheng changed the title [WIP] add ECS support add ECS support Jul 15, 2021
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.

LGTM, one suggestion about removing unused require

require 'logstash/plugin_mixins/ecs_compatibility_support/target_check'
require 'logstash/plugin_mixins/validator_support/field_reference_validation_adapter'
require 'logstash/plugin_mixins/event_support/event_factory_adapter'
require 'logstash/plugin_mixins/event_support/from_json_helper'
Copy link
Contributor

Choose a reason for hiding this comment

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

✂️ we do need the require 'logstash/plugin_mixins/event_support/from_json_helper'

@kaisecheng kaisecheng requested a review from karenzone July 22, 2021 13: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.

Docs: Left some comments inline for your consideration.


Define the target field for placing the row values. If this setting is not
set, the Avro data will be stored at the root (top level) of the event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include this note from the commented code?
Note that the target is only relevant when decoding data into a new event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is important. many thanks

Define the target field for placing the row values. If this setting is not
set, the Avro data will be stored at the root (top level) of the event.

For example, if you want data to be put under the `document` field:
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
For example, if you want data to be put under the `document` field:
*Example*
In this configuration, data will be placed under the `document` field.

}
}
}
----------------------------------
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
----------------------------------
----------------------------------

@kaisecheng kaisecheng merged commit f3e3397 into logstash-plugins:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ECS-Compatibility Mode
4 participants