Normalize the exception behaviour for inputs outputs and filters #2386
Conversation
| @@ -114,4 +130,64 @@ class TestPipeline < LogStash::Pipeline | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| context "when filters raise exceptions" do | |||
There was a problem hiding this comment.
when filters plugins raise exceptions ?
|
Also related to #1373 |
|
First off, thanks for helping start this discussion more visibly outside our little team :)
I'm not in favor of this behavior for two reasons. The first is historical behavior and reason behind it. The second pertains to clustering and the future. First, history. There's tons of external libraries of varying stability (in the library and in the integration with Logstash) that Logstash depends on that mean uncaught exceptions would cause Logstash to terminate. My original idea was that you could write simpler plugins and worry less about intermittent failures (via exceptions) by simply restarting the plugin when it failed. This was most effectively done for the input plugins (because they have an obvious "start" behavior), and less so for filters and outputs. This "restart it when it fails" is common in process managers like daemontools, upstart, systemd, and runit; it also is a central goal of Erlang's supervisor behaviors. Second, future! If we terminate logstash on any uncaught error, then how does this impact things when Logstash is a cluster? I don't think the cluster should terminate with one node having trouble, right? And what of multitenant cluster deployments where one user could accidentally push a bad configuration (or plugin!) that causes errors in that user's log flow - we don't want to interrupt any flows by other tenants. My practice in operations for the past few years has always been to run applications in a kind of "do_thing while true" to restart a thing whenever it exits (on error, or otherwise, always restart it!). In Logstash's case, this follows with my desire to always restart components on failures. For permanent failures, an event could be poisonous and we need to detect these situatinos and direct them to some kind of store for later use (dead letter queue, etc), but the default should be to restart the failed component and only direct any poisonous input (events) do a safe place after several restart attempts have failed. |
jsvd
force-pushed
the
fix/output_thread_lock
branch
from
eb8bbb0 to
1cfa954
Compare
|
test this please |
| @@ -302,4 +295,8 @@ def flush_filters_to_output!(options = {}) | |||
| end | |||
| end # flush_filters_to_output! | |||
|
|
|||
| private | |||
| def print_exception_information(exception) | |||
| @logger.error("Restarting worker: #{exception} => #{exception.backtrace}") | |||
There was a problem hiding this comment.
@logger.error("Exception information", :exception => exception, :backtrace => exception.backtrace)
|
Great stuff here. Thanks, all! |
|
TODO include concern of #1588 |
input, filter and output workers should retry on a StandardError. This PR adds that behaviour and tests. It also changes Thread.abort_on_exception to false so that the pipeline.run method catches the exceptions on other threads through `thread.join`. Otherwise an Exception on a thread directly calls exit and logstash terminates
jsvd
force-pushed
the
fix/output_thread_lock
branch
from
ef560ab to
7ee652d
Compare
| break if event.is_a?(LogStash::ShutdownEvent) | ||
| output(event) | ||
| end # while true | ||
|
|
||
| rescue => e |
|
I'm going to try to test this as I believe I'm having a problem similar to that described in #2130. |
|
See #4064 |
|
Please continue discussion here #4127 |
|
super old stuff. closing |
Right now an exception on an input plugin will cause it to restart indefinitely, while an exception in an output plugin halts logstash immediately.
[EDITED] Taking @jordansissel's comments into account, this pull request makes all plugins have the same behaviour:
If a plugin raises a StandardError, worker catches and retries the method. An Exception makes logstash crash.
This might not be the desired behaviour we want, so this PR is more a WIP, aiming at consistent behaviour across plugins.
Also this adds tests to the pipeline for this behaviour.
Issues related to this problem:
#2152
#2130
#1250
#1373
fixes #2477