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 Compatibility mode #291

Merged
merged 2 commits into from
Apr 30, 2021
Merged

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Apr 23, 2021

Adds an ECS Compatibility mode.

Without this mode, metadata is persisted to each event's top-level host and path fields, but with ECS enabled, it is persisted to the ECS-compatible [host][name] and [log][file][path].

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.

Doc/changelog review: Builds and renders cleanly. Comments inline for your consideration

CHANGELOG.md Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Show resolved Hide resolved
@jsvd
Copy link
Member

jsvd commented Apr 25, 2021

Since the PR already has a review request for @kares I removed elasticsearch-bot as an assignee

@yaauie yaauie removed the request for review from kares April 28, 2021 12:58
@kaisecheng kaisecheng self-requested a review April 28, 2021 14:40
Copy link

@kaisecheng kaisecheng left a comment

Choose a reason for hiding this comment

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

LGTM 🌈 code is clean. CI red is unrelated flaky test

@yaauie yaauie merged commit 9be8dc2 into logstash-plugins:master Apr 30, 2021
@yaauie yaauie deleted the ecs-compatibility branch April 30, 2021 15:38
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.

Late Review: 👍

there's some opportunity for a minor clean (as outlined in the comment),

also while keeping [@metadata][path] and [@metadata][host] makes sense for compatibility and smooth migration I wonder if in case of multiple inputs if it would have been worth also transitioning to a [@metadata][input][file][path] (and [@metadata][input][file][host]) to avoid potential collisions.

@@ -41,7 +41,6 @@ def accept(data)

def process_event(event)
event.set("[@metadata][path]", path)
Copy link
Contributor

Choose a reason for hiding this comment

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

would have been nice to move this final event manipulation part to the file input and simply have:

    def process_event(event)
      input.post_process_this(event, path) # added path argument
    end

... instead of setting meta-data just to retrieve it event.get('[@metadata][path]') right away in post_process_this

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.

7 participants