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

Implement ECS Compatibility Mode #952

Merged
merged 6 commits into from
Jul 14, 2020

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Jul 13, 2020

Implements ECS Compatibility Mode for ECS v1.

  • The templates are imported from elastic/ecs, and targeted to indices matching ecs-logstash-*.
  • Default index and rollover alias names are prepended with ecs- when ECS is enabled.

@yaauie yaauie requested a review from kares July 13, 2020 16:13
@yaauie yaauie force-pushed the ecs-compatibility-mode branch 2 times, most recently from 3e99a5a to 0601fc6 Compare July 13, 2020 18:11
@yaauie
Copy link
Contributor Author

yaauie commented Jul 13, 2020

Remaining test failure is caused by a spec relying the result of a Plugin#register hook before the plugin is sent Plugin#register. I'm moving the hook to be a Plugin#initialize hook instead, which should eliminate the issue.

@kares kares assigned kares and unassigned elasticsearch-bot Jul 14, 2020
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 🐘 🥥 🍶

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

I've made a few minor remarks, mostly around template handling.
One thing that worries me from the user experience side is the consideration that ECS enabled data streams will ship soon, and maybe that should be the default way to push ECS data from logstash.
For other non timeseries/non data streams scenarios it's likely that data won't be ECS formatted.

Rakefile Outdated Show resolved Hide resolved
lib/logstash/outputs/elasticsearch/template_manager.rb Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
By consistently using the Output's already-public method, we can eliminate the
spec's need to specify behaviour of the method internal client, which it can't
reliably intercept in time.
@yaauie yaauie force-pushed the ecs-compatibility-mode branch 2 times, most recently from a198b13 to 53251c8 Compare July 14, 2020 18:35
* Default value is `logstash`
* Default value depends on whether <<plugins-{type}s-{plugin}-ecs_compatibility>> is enabled:
** ECS Compatibility disabled: `logstash`
** ECS Compatibility enabled: `ecs-logstash` (ECS Compatibility is enabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
** ECS Compatibility enabled: `ecs-logstash` (ECS Compatibility is enabled)
** ECS Compatibility enabled: `ecs-logstash`

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.

Nicely done! I made a few suggestion inline. Otherwise, LGTM

docs/index.asciidoc Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
@yaauie yaauie merged commit e7116f4 into logstash-plugins:master Jul 14, 2020
@yaauie yaauie deleted the ecs-compatibility-mode branch July 14, 2020 22:33
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.

5 participants