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

Don't crash on error when send raises #10

Merged
merged 2 commits into from Dec 11, 2019

Conversation

robbavey
Copy link
Contributor

In certain cases (for example, when a UDP payload is too large),
the send method can raise an exception, causing the plugin to crash.

This commit will drop the event and log if this occurs.

In certain cases (for example, when a UDP payload is too large),
the send method can raise an exception, causing the plugin to crash.

This commit will drop the event and log if this occurs.
@elasticsearch-bot elasticsearch-bot self-assigned this Feb 25, 2019
Copy link
Contributor

@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.

LGTM, feel free to bump the patch version + changelog entry and publish a new version

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.

Structure LGTM.

See in-line note about logging the event that we're dropping.

logger.error("Failed to send event, message size of #{payload.size} too long", :error => e.inspect,
:backtrace => e.backtrace.first(10))
rescue => e
logger.error("Failed to send event:", :error => e.inspect,
Copy link
Contributor

Choose a reason for hiding this comment

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

it may be helpful to output at least part of the event in question to the logs if we are effectively dropping it.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I'd suggest having a conditional to log the event if only in debug log level

…enabled

Will output a truncated event payload if debug logging is enabled
@colinsurprenant
Copy link
Contributor

Bump. Thanks @robbavey for this.

This is a nice improvement but it considerably changes the semantic of the plugin which goes from crashing the pipeline upon any socket exception to dropping and moving on to next event in case of exception.

The error handling semantics of output plugins in logstash in not well defined and our plugin behaviors range from crashing the pipeline to dropping event to having a retry policy.

I suggest we move forward with this PR but also

  • add a bounded retry policy for (IOError) Operation not permitted exception as described in (IOError) Operation not permitted exception crashes the plugin #11 (using a max retry with some backoff and crash when max retry is reached).
  • either crash the plugin as before for other exceptions or also retry them with the same retry policy

@colinsurprenant
Copy link
Contributor

Ping @jsvd @yaauie since you have been originally reviewing.

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.

None yet

5 participants