Skip to content

Commit

Permalink
Do now allow DirectFileStore#increment when using :most_recent aggr…
Browse files Browse the repository at this point in the history
…egation

If we do this, we'd be incrementing the value for *this* process, not the
global one, which is almost certainly not what the user wants to do.

This is not very pretty because we may end up raising an exception in
production (as test/dev tend to not use DirectFileStore), but we consider
it better than letting the user mangle their numbers and end up with
incorrect metrics.
  • Loading branch information
Daniel Magliola committed Feb 25, 2020
1 parent 4c23722 commit 6812f38
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 0 deletions.
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,9 @@ Counters, Histograms and Summaries are `SUM`med, and Gauges report all their val
for each process), tagged with a `pid` label. You can also select `SUM`, `MAX`, `MIN`, or
`MOST_RECENT` for your gauges, depending on your use case.

Please note that that the `MOST_RECENT` aggregation only works for gauges, and it does not
allow the use of `increment` / `decrement`, you can only use `set`.

**Memory Usage**: When scraped by Prometheus, this store will read all these files, get all
the values and aggregate them. We have notice this can have a noticeable effect on memory
usage for your app. We recommend you test this in a realistic usage scenario to make sure
Expand Down
6 changes: 6 additions & 0 deletions lib/prometheus/client/data_stores/direct_file_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ def set(labels:, val:)
end

def increment(labels:, by: 1)
if @values_aggregation_mode == DirectFileStore::MOST_RECENT
raise InvalidStoreSettingsError,
"The :most_recent aggregation does not support the use of increment"\
"/decrement"
end

key = store_key(labels)
in_process_sync do
value = internal_store.read_value(key)
Expand Down
12 changes: 12 additions & 0 deletions spec/prometheus/client/data_stores/direct_file_store_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,18 @@
# Both processes should return the same value
expect(metric_store1.all_values).to eq(metric_store2.all_values)
end

it "does now allow `increment`, only `set`" do
metric_store1 = subject.for_metric(
:metric_name,
metric_type: :gauge,
metric_settings: { aggregation: :most_recent }
)

expect do
metric_store1.increment(labels: {})
end.to raise_error(Prometheus::Client::DataStores::DirectFileStore::InvalidStoreSettingsError)
end
end

it "resizes the File if metrics get too big" do
Expand Down

0 comments on commit 6812f38

Please sign in to comment.