AWS SQS input/output plugins #211

Merged
merged 15 commits into from Oct 16, 2012

Conversation

Projects
None yet
2 participants
@organicveggie
Contributor

organicveggie commented Sep 24, 2012

This provides input and output plugins for use with the Simple Queuing Service at Amazon Web Services. Logstash clients can use the output plugins to push log events into an SQS queue, while Logstash servers can pull events from an SQS queue using the input plugin.

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+require "logstash/inputs/threadable"
+require "logstash/namespace"
+
+class LogStash::Inputs::SQS < LogStash::Inputs::Threadable

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Should be some comments above this line that explain what this plugin is for, where to find out more information about SQS, etc.

Example: https://github.com/logstash/logstash/blob/master/lib/logstash/inputs/heroku.rb#L4-17

@jordansissel

jordansissel Sep 24, 2012

Contributor

Should be some comments above this line that explain what this plugin is for, where to find out more information about SQS, etc.

Example: https://github.com/logstash/logstash/blob/master/lib/logstash/inputs/heroku.rb#L4-17

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+
+ # IAM key/secret
+ config :access_key, :validate => :string, :required => true
+ config :secret_key, :validate => :string, :required => true

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

this config option needs a documentation comment like the others.

(clarification: this is necessary because the comments above the config declarations are how logstash generates its plugin reference docs)

@jordansissel

jordansissel Sep 24, 2012

Contributor

this config option needs a documentation comment like the others.

(clarification: this is necessary because the comments above the config declarations are how logstash generates its plugin reference docs)

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ :secret_access_key => @secret_key
+ )
+ begin
+ @logger.debug("Connecting to AWS SQS queue '#{@queue}'...")

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

It's generally better to do structured logging; use a human readable message followed by some attributes:

@logger.debug("Connecting to AWS SQS queue", :queue => @queue)
@jordansissel

jordansissel Sep 24, 2012

Contributor

It's generally better to do structured logging; use a human readable message followed by some attributes:

@logger.debug("Connecting to AWS SQS queue", :queue => @queue)
@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ begin
+ @logger.debug("Connecting to AWS SQS queue '#{@queue}'...")
+ @sqs_queue = @sqs.queues.named(@queue)
+ @logger.info("Connected to AWS SQS queue '#{@queue}' successfully.")

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

same comment here about structured logs vs plain text, something like @logger.info("Connected successfully to AWS SQS queue", :queue => @queue)

@jordansissel

jordansissel Sep 24, 2012

Contributor

same comment here about structured logs vs plain text, something like @logger.info("Connected successfully to AWS SQS queue", :queue => @queue)

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ @logger.info("Connected to AWS SQS queue '#{@queue}' successfully.")
+ rescue Exception => e
+ @logger.error("Unable to access SQS queue '#{@queue}': #{e.to_s}")
+ throw e

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

raise, not throw ;)

'throw' is similar but different in ruby.

@jordansissel

jordansissel Sep 24, 2012

Contributor

raise, not throw ;)

'throw' is similar but different in ruby.

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ @logger.debug("Processed SQS message #{message.id} [#{message.md5}] from queue '#{@queue}'")
+ output_queue << e
+ message.delete
+ end

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Generally for long indented blocks, I like to have comments after the 'end' detailing what block is being ended. See examples at the end of the sample here: https://github.com/logstash/logstash/blob/master/STYLE.md

Real-world example: https://github.com/logstash/logstash/blob/master/lib/logstash/filters/date.rb#L120-122

@jordansissel

jordansissel Sep 24, 2012

Contributor

Generally for long indented blocks, I like to have comments after the 'end' detailing what block is being ended. See examples at the end of the sample here: https://github.com/logstash/logstash/blob/master/STYLE.md

Real-world example: https://github.com/logstash/logstash/blob/master/lib/logstash/filters/date.rb#L120-122

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ begin
+ yield
+ rescue AWS::EC2::Errors::RequestLimitExceeded
+ puts "AWS::EC2::Errors::RequestLimitExceeded ... retrying #{message} in #{sleep_time} seconds"

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Perhaps @logger.warn here instead of 'puts'

@jordansissel

jordansissel Sep 24, 2012

Contributor

Perhaps @logger.warn here instead of 'puts'

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ end
+
+ begin
+ yield

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

I prefer 'block.call' instead of yield since it's more explicit, and from the function definition you can see it uses &block and you can search down for where block used.

@jordansissel

jordansissel Sep 24, 2012

Contributor

I prefer 'block.call' instead of yield since it's more explicit, and from the function definition you can see it uses &block and you can search down for where block used.

@jordansissel

View changes

lib/logstash/inputs/sqs.rb
+ print "Error for #{message}: #{bang}"
+ return false
+ end
+ true

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Explicit return is preferred: use 'return true'

@jordansissel

jordansissel Sep 24, 2012

Contributor

Explicit return is preferred: use 'return true'

@jordansissel

View changes

lib/logstash/outputs/sqs.rb
+require "logstash/outputs/base"
+require "logstash/namespace"
+
+class LogStash::Outputs::SQS < LogStash::Outputs::Base

This comment has been minimized.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Same basic review on this plugin as your input plugin. Documentation comments, some style fixes, etc.

@jordansissel

jordansissel Sep 24, 2012

Contributor

Same basic review on this plugin as your input plugin. Documentation comments, some style fixes, etc.

jordansissel added a commit that referenced this pull request Oct 16, 2012

Merge pull request #211 from StudyBlue/sqs
AWS SQS input/output plugins

@jordansissel jordansissel merged commit 2a05401 into elastic:master Oct 16, 2012

@jordansissel

This comment has been minimized.

Show comment
Hide comment
@jordansissel

jordansissel Oct 16, 2012

Contributor

Thanks! :)

Contributor

jordansissel commented Oct 16, 2012

Thanks! :)

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