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

Added support for DLQ #110

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

RashmiRam
Copy link

  1. Used the same logic as elasticsearch output plugin to find out if dlq is enabled or not
    [execution_context? && execution_context.dlq_writer? && execution_context.dlq_writer is not a dummy writer?]

  2. if dlq is enabled, send it to dlq_writer or log and drop the the events otherwise.

Fixes #109

Signed-off-by: RashmiRam [email protected]

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

1. Used the same logic as elasticsearch output plugin to find out if dlq is enabled or not
[execution_context? && execution_context.dlq_writer? && execution_context.dlq_writer is not a dummy writer?]

2. if dlq is enabled, send it to dlq_writer or log and drop the the events otherwise.

Fixes logstash-plugins#109

Signed-off-by: RashmiRam <[email protected]>
@cla-checker-service
Copy link

cla-checker-service bot commented Mar 23, 2020

💚 CLA has been signed

Signed-off-by: RashmiRam <[email protected]>
@RashmiRam
Copy link
Author

@jsvd Could you please review this when you have time?

Copy link

@andsel andsel left a comment

Choose a reason for hiding this comment

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

Hi @RashmiRam sorry for very late review.
First of all, thank you a lot for this contribution!

I've left some comments.

I think that before moving forward the code base has to be re-aligned to main, to resolve some merge conflicts.

Comment on lines +169 to +172
metadata = event.get("@metadata")
metadata.each_pair do |key, value|
event.set("[custom_metadata][#{key}]", value)
end
Copy link

Choose a reason for hiding this comment

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

@metadata field is already stored in the DLQ event, so that it can be read back on the downstream DLQ consumer.
No need to copy the @metadata in custom_metadata field before DLQ write.

@@ -60,6 +60,9 @@ class LogStash::Outputs::Http < LogStash::Outputs::Base
# If encountered as response codes this plugin will retry these requests
config :retryable_codes, :validate => :number, :list => true, :default => [429, 500, 502, 503, 504]

# If encountered as response codes, this plugin will write these events to DLQ
config :dlq_retryable_codes, :validate => :number, :list => true, :default => [400, 403, 404, 401]
Copy link

Choose a reason for hiding this comment

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

Suggested change
config :dlq_retryable_codes, :validate => :number, :list => true, :default => [400, 403, 404, 401]
config :dlq_retryable_codes, :validate => :number, :list => true, :default => [400, 401, 403, 404]

@@ -1,6 +1,6 @@
Gem::Specification.new do |s|
s.name = 'logstash-output-http'
s.version = '5.2.4'
s.version = '5.2.5'
Copy link

Choose a reason for hiding this comment

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

This is a new feature so it deserve a minor version. Actually main has 5.5.0 so the target could be 5.6.0

Suggested change
s.version = '5.2.5'
s.version = '5.6.0'

@@ -1,3 +1,6 @@
## 5.2.5
Copy link

Choose a reason for hiding this comment

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

Suggested change
## 5.2.5
## 5.6.0

As per later comment

@RashmiRam
Copy link
Author

@andsel Thank you for the review. I will have some time this weekend and will try to address all the comments.

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

Successfully merging this pull request may close these issues.

DLQ support
3 participants