Closed cyx closed 7 years ago
I think the definition of empty?
on Librato::Metrics::Queue
also needs to change, to look something like this:
def empty?
gauges.empty? && counters.empty? && measurements.empty?
end
Hi @cyx. Thanks for the report and contribution.
Currently it's checking queued.empty? which always turns out to be non-empty if you provide a source.
Could you elaborate on this with an example to reproduce? Seems like the expected behavior:
q1 = Librato::Metrics::Queue.new
q1.empty?
# => true
q1.add foo: { value: 1, source: 'bar' }
q1.queued
# => {:gauges=>[{:value=>1, :source=>"bar", :name=>"foo", :measure_time=>1489707683}]}
q1.empty?
# => false
q2 = Librato::Metrics::Queue.new(source: 'test')
q2.empty?
# => true
q2.add foo: { value: 1, source: 'bar' }
q2.queued
# => {:gauges=>[{:value=>1, :source=>"bar", :name=>"foo", :measure_time=>1489707741}], :source=>"test"}
q2.empty?
# => false
I'm afk now, will try to provide tomorrow.
@chancefeick here are the steps to reproduce:
> aggregator = Librato::Metrics::Aggregator.new
> queue = Librato::Metrics::Queue.new(source: 'test')
> queue.empty?
=> true
> queue.merge!(aggregator)
> queue.empty?
=> false
> queue.queued
=> {:gauges=>[], :source=>"test"}
Thanks for the example, that's definitely not the intended behavior.
These changes make sense to me 👍 Can you please include specs around this fix so we can ensure it doesn't unintentionally break in a future version?
@cyx I came up with the following. If you think it looks good you can just add it to your branch:
diff --git a/spec/unit/metrics/queue_spec.rb b/spec/unit/metrics/queue_spec.rb
index 163f34f..ea874ff 100644
--- a/spec/unit/metrics/queue_spec.rb
+++ b/spec/unit/metrics/queue_spec.rb
@@ -301,6 +301,11 @@ module Librato
subject.add foo: {type: :gauge, value: 121212}
expect(subject.empty?).to be false
end
+
+ it "returns true when nothing merged" do
+ subject.merge!(Librato::Metrics::Aggregator.new)
+ expect(subject.empty?).to be true
+ end
end
describe "#gauges" do
@@ -544,6 +549,14 @@ module Librato
expect(subject.queued).not_to be_empty
end
end
+
+ context "when there are no metrics" do
+ it "it does not persist and returns true" do
+ subject.merge!(Librato::Metrics::Aggregator.new)
+ subject.persister.return_value(false)
+ expect(subject.submit).to be true
+ end
+ end
end
describe "#time" do
@mikehale thanks! Pushed the commit.
@chancefeick can you check and see if it's all good now?
@cyx LGTM 👍
Released in v2.1.1. Thanks again for your contribution.
Thanks!
Currently it's checking queued.empty? which always turns out to be non-empty if you provide a source.
This check relies on just
empty?
which only utilizes@queued
without the merging of the global metadata.