Skip to content

Feat: adjust fields for ECS compatibility #28

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

Merged
merged 14 commits into from
Nov 22, 2021
Merged

Conversation

kares
Copy link
Contributor

@kares kares commented Nov 16, 2021

Since the plugin was setting fields that conflict with ECS schema, we're introducing an ecs_compatibility mode.

  • legacy host becomes [host][name] in ECS mode
  • the socket file path is now [file][path] in ECS mode

Also, fixed i-var name and logged message as reported in #25

NOTE: CI failures are not new - 6.x has been broken for a long time.
The problematic spec assumes a client socket blocking from readpartial (while the server keep the socket open wout writing anything) to be interruptible on client_socket.close - this is not the case and the only reason the spec does not hang CI is due the teardown closing the server socket -> leads to a EOF from the readpartial.
Also the potential use of timeout is problematic - a proper implementation would be to use select with a timeout and check for stopping periodically.

@kares kares marked this pull request as ready for review November 16, 2021 13:30
@kares kares requested review from karenzone and yaauie November 16, 2021 14:11
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.

Left some suggestion inline for your consideration. Version bump will be required to get these changes picked up and added to the documentation. Thanks!

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

The ECS-related changes LGTM, so in that since I'm approving this PR.

I also appreciated your analysis on why this has been failing CI in 6.x for a long time. I agree that using IO::select with a timeout is a less-troublesome approach than Timeout::timeout, and it would be nice to fix the specs for 6.x before 6.x is EOL'd. Can this be done in a follow-up effort?

@kares kares merged commit 6f8d7f7 into logstash-plugins:main Nov 22, 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.

adapt field names for ECS compatibility Error message prints non-existing variable
3 participants