Skip to content

Add pipeline.continue_on_error setting to allow logstash to rescue arbitrary…#5562

Closed
andrewvc wants to merge 1 commit intoelastic:masterfrom
andrewvc:rescue-pipeline-failures
Closed

Add pipeline.continue_on_error setting to allow logstash to rescue arbitrary…#5562
andrewvc wants to merge 1 commit intoelastic:masterfrom
andrewvc:rescue-pipeline-failures

Conversation

@andrewvc
Copy link
Copy Markdown
Contributor

@andrewvc andrewvc commented Jun 28, 2016 •
edited
Loading

This will rescue any plugin or pipeline config lang errors. This setting is only settable in the YAML.

Fixes #5547 and #1637

@andrewvc andrewvc self-assigned this Jun 28, 2016
@andrewvc andrewvc force-pushed the rescue-pipeline-failures branch from 782fdf4 to f1f06e7 Compare June 28, 2016 18:24
jordansissel
jordansissel reviewed Jun 28, 2016
View reviewed changes
Comment thread config/logstash.yml Outdated
#
# pipeline.unsafe_shutdown: false
#
# Defaults to false. Setting this to true will rescue all exceptions in the
Copy link
Copy Markdown
Contributor

@jordansissel jordansissel Jun 28, 2016

Choose a reason for hiding this comment

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

"rescue" is a ruby-specific term and 'exceptions' feels programmer-centric

Being that this should target users/operators, are there any better ways to describe this? "Unexpected errors" or something like that?

@ph
Copy link
Copy Markdown
Contributor

ph commented Jun 28, 2016

Code LGTM, I think @jordansissel's comment concerning the doc is valid and should be addressed before we merge this.

@andrewvc andrewvc force-pushed the rescue-pipeline-failures branch from f1f06e7 to 61c6b6d Compare June 28, 2016 18:39
@andrewvc
Copy link
Copy Markdown
Contributor Author

andrewvc commented Jun 28, 2016

@ph @jordansissel I have pushed a new version that should address your critiques.

@ph
Copy link
Copy Markdown
Contributor

ph commented Jun 28, 2016

build failed?

--- jar coordinate com.fasterxml.jackson.core:jackson-databind already loaded with version 2.7.1 - omit version 2.7.1-1
rake aborted!
SyntaxError: /home/travis/build/elastic/logstash/logstash-core/lib/logstash/pipeline.rb:326: syntax error, unexpected '\n'
/home/travis/build/elastic/logstash/logstash-core/lib/logstash/agent.rb:10:in `(root)'
/home/travis/build/elastic/logstash/vendor/bundle/jruby/1.9/gems/logstash-devutils-1.0.2-java/lib/logstash/devutils/rspec/logstash_helpers.rb:1:in `(root)'
/home/travis/build/elastic/logstash/vendor/bundle/jruby/1.9/gems/logstash-devutils-1.0.2-java/lib/logstash/devutils/rspec/logstash_helpers.rb:1:in `(root)'
/home/travis/build/elastic/logstash/vendor/bundle/jruby/1.9/gems/logstash-devutils-1.0.2-java/lib/logstash/devutils/rspec/spec_helper.rb:1:in `(root)'
/home/travis/build/elastic/logstash/vendor/bundle/jruby/1.9/gems/logstash-devutils-1.0.2-java/lib/logstas

@andrewvc andrewvc force-pushed the rescue-pipeline-failures branch from 61c6b6d to 79f55b5 Compare June 28, 2016 18:51
ph
ph reviewed Jun 28, 2016
View reviewed changes
Comment thread logstash-core/lib/logstash/pipeline.rb

if @logger.info?
logger_opts[:events] = Array(event_or_events).map(&:to_hash)
end
Copy link
Copy Markdown
Contributor

@ph ph Jun 28, 2016

Choose a reason for hiding this comment

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

Maybe if debug? == true add stacktraces?

Copy link
Copy Markdown
Contributor

@ph ph Jun 28, 2016

Choose a reason for hiding this comment

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

Do we have any concerns here about sensitives data that could be leak here?

@andrewvc andrewvc force-pushed the rescue-pipeline-failures branch from 79f55b5 to 9873201 Compare June 28, 2016 20:24
@andrewvc andrewvc force-pushed the rescue-pipeline-failures branch from 9873201 to 5f7a4b3 Compare June 28, 2016 20:25
@andrewvc andrewvc changed the title Add pipeline.rescue_all setting to allow logstash to rescue arbitrary… Add pipeline.continue_on_error setting to allow logstash to rescue arbitrary… Jun 28, 2016
ph
ph reviewed Jun 28, 2016
View reviewed changes
Comment thread logstash-core/spec/logstash/pipeline_spec.rb
end
end
end
end
Copy link
Copy Markdown
Contributor

@ph ph Jun 28, 2016

Choose a reason for hiding this comment

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

that was easy (tm)

@ph
Copy link
Copy Markdown
Contributor

ph commented Jun 28, 2016

LGTM

@suyograo
Copy link
Copy Markdown
Contributor

suyograo commented Jul 5, 2016

Can we mark this as being experimental in the config docs?

@andrewvc andrewvc mentioned this pull request Jul 6, 2016
Closed
@andrewvc
Copy link
Copy Markdown
Contributor Author

andrewvc commented Jul 7, 2016

Will rename this to continue_on_conditional_error after closing #5593 , then this setting will only rescue nil check errors in conditionals

@jsvd jsvd self-assigned this Jul 11, 2016
@jsvd jsvd mentioned this pull request Jul 15, 2016
Closed
@jsvd jsvd added v5.0.0 and removed v5.0.0-beta1 labels Sep 13, 2016
@suyograo
Copy link
Copy Markdown
Contributor

suyograo commented Oct 5, 2016

We discussed this internally, and it will be handled in the DLQ feature. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jsvd jsvd

@andrewvc andrewvc

Labels

v5.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@andrewvc @ph @suyograo @jordansissel @jsvd