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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

andrewvc
Copy link
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
jordansissel
jordansissel reviewed Jun 28, 2016
View reviewed changes
config/logstash.yml
@@ -50,6 +50,16 @@
#
# pipeline.unsafe_shutdown: false
#
# Defaults to false. Setting this to true will rescue all exceptions in the
Copy link
Contributor

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
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
Copy link
Contributor Author

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

@ph
Copy link
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

ph
ph reviewed Jun 28, 2016
View reviewed changes
logstash-core/lib/logstash/pipeline.rb

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

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
Contributor

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 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
logstash-core/spec/logstash/pipeline_spec.rb
end
end
end
end
Copy link
Contributor

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
Contributor

ph commented Jun 28, 2016

LGTM

@suyograo
Copy link
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
Open
@andrewvc
Copy link
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
Contributor

suyograo commented Oct 5, 2016

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

@suyograo suyograo closed this Oct 5, 2016
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