Add log devices and write to all of them #1
Conversation
I've been trying to do everything in 1.9 first, but it turns out there's still some 1.8 stuff that I need to support that I'd like to use this with.
Thanks to Mahlon for spotting this and helping to track it down.
| target_regex = /^([\sa-z]\w*)(?:\[(.*)\])?/ | ||
|
|
||
| target_subclass = t[ target_regex, 1 ]&.strip().to_sym | ||
| target_subclass_args = t[ target_regex, 2 ] |
There was a problem hiding this comment.
Should we nil-check the target_subclass_args? Again, as an example, as written something like data-dog[foo_key] will return a target_subclass of data and a target_subclass_args of nil.
There was a problem hiding this comment.
Not all log devices need target_subclass_args e.g. the Appending log device class does not require an argument.
lib/loggability/log_device.rb
Outdated
| ### In the above example: | ||
| ### "datadog" is the log device to send logs to | ||
| ### "[data_dog_api_key]" is the argument that will be passed onto the datadog subclass | ||
| ### "(color)" is the format |
There was a problem hiding this comment.
I don't see how the format in this example gets used?
There was a problem hiding this comment.
It doesn't it's just there to show what a config entry looks like. I will remove it.
lib/loggability/log_device.rb
Outdated
| ### datadog[data_dog_api_key] (color) | ||
| ### In the above example: | ||
| ### "datadog" is the log device to send logs to | ||
| ### "[data_dog_api_key]" is the argument that will be passed onto the datadog subclass |
There was a problem hiding this comment.
This is a little unclear to me - what do you think about these changes?
"data_dog_api_key" is the argument that will be passed onto the datadog log device's constructor
(Hash part of the string isn't actually provided, and making it clearer what it's passed into)
lib/loggability/http_log_device.rb
Outdated
|
|
||
| require 'loggability/log_device' | ||
|
|
||
| # This is the a generalized class that allows its subclasses send |
There was a problem hiding this comment.
missing a "to" - "subclasses send to". Also, maybe some specific mention of running on a separate thread?
| }) | ||
|
|
||
| ## send the batch of logs if it is approaching datadog's payload size limits | ||
| if self.logs_queue.size == MAX_BATCH_SIZE || self.batched_messages_bytesize > MAX_BATCH_BYTESIZE |
There was a problem hiding this comment.
We should check >= MAX_BATCH_SIZE rather than exact equality, just to be resilient to usages where multiple pushs happen before this check
There was a problem hiding this comment.
and may as well make the MAX_BATCH_BYTESIZE check a >= for consistency, too
| attr_reader :batched_messages_bytesize | ||
|
|
||
|
|
||
| ### Sets up the data for the log message and sends it to datadog |
There was a problem hiding this comment.
Maybe something like "and sends it to datadog if the queue is full"?
| ### | ||
| ### sends all a batch of up to the batched log messages asynchronously to datadog | ||
| def send_logs | ||
| return if self.logs_queue.size == 0 |
| return if self.logs_queue.size == 0 | ||
|
|
||
| batched_messages = Concurrent::Array.new | ||
| while self.logs_queue.size > 0 |
There was a problem hiding this comment.
Just a nitpicky suggestion for readability, but I like using the empty? semantics where it's available, so just throwing this out there:
batched_messages.push( self.logs_queue.pop ) until self.logs_queue.empty?
| def send_logs | ||
| return if self.logs_queue.size == 0 | ||
|
|
||
| batched_messages = Concurrent::Array.new |
There was a problem hiding this comment.
I don't think we need this array to be thread-safe, right? It's going to be allocated, filled and read all within a single thread with a scope local to this method. Sometimes I get my wired crossed with this stuff though.
|
|
||
|
|
||
| ### Queue is a thread-safe and it is used to batch log messages | ||
| def setup_logs_queue |
There was a problem hiding this comment.
Can we just inline this into the constructor? Looks like it's only called there.
spec/loggability/logger_spec.rb
Outdated
| @@ -176,7 +223,7 @@ | |||
| expect( logger.logdev ).to be( original_logdev ) | |||
| end | |||
|
|
|||
| it "doesn't re-wrap an AppendingLogDevice" do | |||
| it "doesn't re-wrap an Appending" do | |||
spec/loggability/logger_spec.rb
Outdated
| with( datadog_logdev.target ). | ||
| and_return( Net::HTTPOK ) | ||
|
|
||
| 500.times do |
There was a problem hiding this comment.
Rather than hard-code this number again, we should use MAX_BATCH_SIZE
spec/loggability/logger_spec.rb
Outdated
| expect( datadog_logdev.http ).not_to receive( :request ). | ||
| with( datadog_logdev.target ) | ||
|
|
||
| 499.times do |
spec/loggability/logger_spec.rb
Outdated
| expect( logger.logdev ).to be_a( Logger::LogDevice ) | ||
| end | ||
|
|
||
| it "can be told to log to datadog and not send messages" do |
There was a problem hiding this comment.
This spec description to me reads like you can configure the datadog logger to not send messages, but what's being tested is that it doesn't post until hitting the batch size. Would you mind adding something about the batch size to these two descriptions?
lib/loggability/spechelpers.rb
Outdated
| @@ -7,7 +7,7 @@ | |||
| # Some helper functions for testing. Usage: | |||
| # | |||
| # # in spec_helpers.rb | |||
| # RSpec.configure do |c| | |||
| # igure do |c| | |||
There was a problem hiding this comment.
oops, not sure if we should just delete this whole comment, but some accidentally deleted text
spec/helpers.rb
Outdated
| @@ -112,5 +111,11 @@ def all_be_a( expected_class ) | |||
| c.include( Loggability::SpecHelpers ) | |||
| c.filter_run_excluding( :configurability ) unless defined?( Configurability ) | |||
|
|
|||
| # c.before(:each) do | |||
| end | ||
|
|
||
|
|
||
| it "verify that the http client is setup correctly" do |
There was a problem hiding this comment.
I don't think we want to test for specific constant values. IMO, Configurations and constants should be mutable, and the tests should not make assumptions about what the values are. Having this tests just means we have to modify constants in two places instead of one.
There was a problem hiding this comment.
Just to elaborate, I think the tests below that e.g. the configured batch size is respected when posting to datadog, is valuable, but testing the exact batch size value isn't.
There was a problem hiding this comment.
I put these specs in there to prevent someone from changing these values to above the datadog's payload limits and not realizing it. But I understand your point. I will just put a note on those constants.
spec/loggability/log_device_spec.rb
Outdated
| end | ||
|
|
||
|
|
||
| it "can prase a config entry and return a Loggability::LogDevice::File log instance" do |
mahmoodkhan
force-pushed
the
log-to-multiple-devices
branch
2 times, most recently
from
f995446
to
92fcf54
Compare
| @@ -28,7 +28,6 @@ | |||
| require 'loggability' | |||
| require 'loggability/spechelpers' | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
looks like some extra spacing changes made it in
mahmoodkhan
force-pushed
the
log-to-multiple-devices
branch
2 times, most recently
from
0a4338d
to
a93802b
Compare
sending to http log devices
mahmoodkhan
force-pushed
the
log-to-multiple-devices
branch
from
a93802b
to
ff08c7f
Compare
|
I imported this into the canonical repo, and will finish it up in the next few days |
|
Also of note: the current revisions in Mercurial look a little weird because I had to split some Ruby 2.7-compatibility stuff away from the patch, and ran into a few conflicts. I'll fix it up before the next push. |
This PR adds the ability to write to more than one log device at the same time. It also adds a datadog log device that sends logs to the datadog service using its HTTP endpoints.
In the next PR, I will change sending logs to datadog non-blocking by spawning a new thread.