Skip to content

Commit f40a541

Browse files
author
Daniel Magliola
committed
Only allow Gauges to use :most_recent aggregation
Using this aggregation with any other metric type is almost certainly not what the user intended, and it'll result in counters that go up and down, and completely inconsistent histograms. Signed-off-by: Daniel Magliola <danielmagliola@gocardless.com>
1 parent 4bb5b4c commit f40a541

2 files changed

Lines changed: 37 additions & 3 deletions

File tree

lib/prometheus/client/data_stores/direct_file_store.rb

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def for_metric(metric_name, metric_type:, metric_settings: {})
4545
end
4646

4747
settings = default_settings.merge(metric_settings)
48-
validate_metric_settings(settings)
48+
validate_metric_settings(metric_type, settings)
4949

5050
MetricStore.new(metric_name: metric_name,
5151
store_settings: @store_settings,
@@ -54,7 +54,7 @@ def for_metric(metric_name, metric_type:, metric_settings: {})
5454

5555
private
5656

57-
def validate_metric_settings(metric_settings)
57+
def validate_metric_settings(metric_type, metric_settings)
5858
unless metric_settings.has_key?(:aggregation) &&
5959
AGGREGATION_MODES.include?(metric_settings[:aggregation])
6060
raise InvalidStoreSettingsError,
@@ -65,6 +65,11 @@ def validate_metric_settings(metric_settings)
6565
raise InvalidStoreSettingsError,
6666
"Only :aggregation setting can be specified"
6767
end
68+
69+
if metric_settings[:aggregation] == MOST_RECENT && metric_type != :gauge
70+
raise InvalidStoreSettingsError,
71+
"Only :gauge metrics support :most_recent aggregation"
72+
end
6873
end
6974

7075
class MetricStore

spec/prometheus/client/data_stores/direct_file_store_spec.rb

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
it_behaves_like Prometheus::Client::DataStores
1616

17-
it "only accepts valid :aggregation as Metric Settings" do
17+
it "only accepts valid :aggregation values as Metric Settings" do
1818
expect do
1919
subject.for_metric(:metric_name,
2020
metric_type: :counter,
@@ -26,14 +26,43 @@
2626
metric_type: :counter,
2727
metric_settings: { aggregation: :invalid })
2828
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
29+
end
2930

31+
it "only accepts valid keys as Metric Settings" do
32+
# the only valid key at the moment is :aggregation
3033
expect do
3134
subject.for_metric(:metric_name,
3235
metric_type: :counter,
3336
metric_settings: { some_setting: true })
3437
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
3538
end
3639

40+
it "only accepts :most_recent aggregation for gauges" do
41+
expect do
42+
subject.for_metric(:metric_name,
43+
metric_type: :gauge,
44+
metric_settings: { aggregation: Prometheus::Client::DataStores::DirectFileStore::MOST_RECENT })
45+
end.not_to raise_error
46+
47+
expect do
48+
subject.for_metric(:metric_name,
49+
metric_type: :counter,
50+
metric_settings: { aggregation: Prometheus::Client::DataStores::DirectFileStore::MOST_RECENT })
51+
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
52+
53+
expect do
54+
subject.for_metric(:metric_name,
55+
metric_type: :histogram,
56+
metric_settings: { aggregation: Prometheus::Client::DataStores::DirectFileStore::MOST_RECENT })
57+
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
58+
59+
expect do
60+
subject.for_metric(:metric_name,
61+
metric_type: :summary,
62+
metric_settings: { aggregation: Prometheus::Client::DataStores::DirectFileStore::MOST_RECENT })
63+
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
64+
end
65+
3766
it "raises when aggregating if we get to that that point with an invalid aggregation mode" do
3867
# This is basically just for coverage of a safety clause that can never be reached
3968
allow(subject).to receive(:validate_metric_settings) # turn off validation

0 commit comments

Comments
 (0)