dwbutler / logstash-logger

Ruby logger that writes logstash events
MIT License
456 stars 118 forks source link

Add support for AWS Firehose #121

Closed lanxx019 closed 7 years ago

lanxx019 commented 7 years ago

AWS Firehose allow direct streaming data to AWS elastic search service. This would allow a seamless delivery of log from hosting machine to ES.

This is mostly mimic what the Kinesis device is doing.

dwbutler commented 7 years ago

Looks good. A few things I would change:

lanxx019 commented 7 years ago

@dwbutler I change the code a bit. Ready for next round.

lanxx019 commented 7 years ago

@dwbutler comment addressed, ready for new round.

lanxx019 commented 7 years ago

Also, do I need to care about the codeclimate warning/errors?

dwbutler commented 7 years ago

@lanxx019 If you can make recoverable_error_codes a class instance variable, I'll be happy. Even though you made it a constant, you still assign it to an instance variable each time the logger class is initialized.

Code Climate warnings are advisory so use your judgement. I didn't see any red flags. Code Climate just didn't understand what you were doing with the abstract base class.

lanxx019 commented 7 years ago

@dwbutler thanks for the review. Sorry I did not know about class instance variable in ruby. I am still live in Java world. I did however, change both recoverable_error_codes and stream_class to be class instance variable. Take a look and let me know if it is OK to merge.

lanxx019 commented 7 years ago

@dwbutler do you have any suggestion on this or is it good to be shipped?

dwbutler commented 7 years ago

@lanxx019 It's good to go. Thanks!

lanxx019 commented 7 years ago

@dwbutler I do not have permission to merge? Could you merge it? Thank you.

dwbutler commented 7 years ago

Released in 0.24.0.