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

Add support for logging to AWS Kinesis (basically a hosted Kafka) #111

Merged
merged 4 commits into from
Jan 6, 2017

Conversation

aianus
Copy link
Contributor

@aianus aianus commented Dec 30, 2016

@dwbutler
Copy link
Owner

Looks good, thanks for the contribution!

@aianus
Copy link
Contributor Author

aianus commented Jan 3, 2017

Any idea about these test failures? They seem unrelated to the patch.

Also, is it ok to just add failed records back to the queue this way when the buffer has sync=true? No deadlock risk? Or should I only add them back to the queue when sync=false?

@dwbutler
Copy link
Owner

dwbutler commented Jan 3, 2017

The test failures are due to a development dependency issue with the latest release of Nokogiri. It's resolved in master, so if you rebase the build failures should go away.

It should be safe to add records back to the buffer this way. Even with sync=true, the exact same buffering mechanism is in use. The only difference is that a flush is forced every time a message is written. While a flush is in progress, messages can still come into the buffer and get queued up. They will get wait there and get dequeued during the next round of flushing.

I think this is good to go, and hope to merge/release it this week.

aianus and others added 2 commits January 3, 2017 13:33
Resolve nokogiri development dependency issue
@aianus aianus force-pushed the aianus/kinesis_device branch from 0111eaf to 987a2ff Compare January 3, 2017 18:33
@aianus
Copy link
Contributor Author

aianus commented Jan 3, 2017

Ok, rebased and updated PR.

@codecov-io
Copy link

codecov-io commented Jan 3, 2017

Current coverage is 91.48% (diff: 92.95%)

Merging #111 into master will increase coverage by 0.14%

@@             master       #111   diff @@
==========================================
  Files            59         61     +2   
  Lines          1408       1479    +71   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1286       1353    +67   
- Misses          122        126     +4   
  Partials          0          0          

Powered by Codecov. Last update 5874e07...5676059

@dwbutler
Copy link
Owner

dwbutler commented Jan 3, 2017

I noticed something missing from this device which is present in other devices. If there is an unrecoverable connection error, other devices will call close(flush: false) to clean up the IO object without flushing any messages. This prevents connections being leaked if there is extended downtime or perhaps a configuration issue. This is the only change I would make. I can totally do it after merging unless you would like to do it.

@dwbutler
Copy link
Owner

dwbutler commented Jan 3, 2017

It would also be a good idea to override close! unless Aws::Kinesis::Client responds to close. (It's hard to tell from the aws-sdk documentation whether it does or not.)

@aianus
Copy link
Contributor Author

aianus commented Jan 4, 2017

I'll take a look and update the PR first thing in the morning.

@dwbutler dwbutler merged commit ca5e395 into dwbutler:master Jan 6, 2017
@dwbutler
Copy link
Owner

dwbutler commented Jan 6, 2017

Released in 0.22.0.

@ashramsey
Copy link

ashramsey commented Mar 24, 2017

Hey guys, @aianus, @dwbutler If I set sync: true, is this supposed to start pushing all logging to my kinesis stream? If so, I'm not able to get it working locally on my dev machine. I can create a LogStashLogger::Device::Kinesis object in rails console and call :write_one to my stream, and that works fine. Wondering what Im doing wrong.

Thanks,

Ash

my development.rb file:

  config.log_level = :debug
  config.logstash.type = :kinesis
  config.logstash.sync = true
  config.logstash.stream = 'pasiphae-logs'
  config.logstash.aws_region = 'us-east-1'
  config.logstash.aws_access_key_id = ENV['AWS_ACCESS_KEY_ID']
  config.logstash.aws_secret_access_key = ENV['AWS_SECRET_ACCESS_KEY']

@dwbutler
Copy link
Owner

dwbutler commented Apr 1, 2017

@ashramsey That's unusual. Yes, the expected behavior when setting sync: true is that every log message is immediately written to the device. For buffered devices, the buffer is still used, but is autoflushed after each write. For debugging, it will help if you set the buffer_logger option. If you're still having trouble with this, feel free to open a new ticket.

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

Successfully merging this pull request may close these issues.

4 participants