diff --git a/lib/prometheus/client/data_stores/direct_file_store.rb b/lib/prometheus/client/data_stores/direct_file_store.rb index 05a506c6..e2533efc 100644 --- a/lib/prometheus/client/data_stores/direct_file_store.rb +++ b/lib/prometheus/client/data_stores/direct_file_store.rb @@ -80,6 +80,7 @@ def initialize(metric_name:, store_settings:, metric_settings:) @store_settings = store_settings @values_aggregation_mode = metric_settings[:aggregation] @store_opened_by_pid = nil + @store_keys = {} @lock = Monitor.new end @@ -127,17 +128,19 @@ def get(labels:) def all_values stores_data = Hash.new{ |hash, key| hash[key] = [] } + # Every process's file tends to hold the same keys, so we decode each + # key only once, rather than once per file. + label_sets = Hash.new do |hash, labelset_qs| + hash[labelset_qs] = parse_labelset(labelset_qs) + end + # There's no need to call `synchronize` here. We're opening a second handle to # the file, and `flock`ing it, which prevents inconsistent reads stores_for_metric.each do |file_path| begin store = FileMappedDict.new(file_path, true) store.all_values.each do |(labelset_qs, v, ts)| - # Labels come as a query string - # "foo=bar&x=y" => { foo: "bar", x: "y" } - label_set = URI.decode_www_form(labelset_qs).to_h.transform_keys(&:to_sym) - - stores_data[label_set] << [v, ts] + stores_data[label_sets[labelset_qs]] << [v, ts] end ensure store.close if store @@ -160,11 +163,26 @@ def in_process_sync def store_key(labels) if @values_aggregation_mode == ALL labels[:pid] = process_id + return encode_labelset(labels) end + # The same label sets get written over and over, so remember their keys. + # We key the cache on a frozen copy, as callers may mutate their hash later. + in_process_sync do + @store_keys[labels] || (@store_keys[labels.dup.freeze] = encode_labelset(labels)) + end + end + + def encode_labelset(labels) URI.encode_www_form(labels.to_a.sort) end + # Labels come as a query string + # "foo=bar&x=y" => { foo: "bar", x: "y" } + def parse_labelset(labelset_qs) + URI.decode_www_form(labelset_qs).to_h.transform_keys(&:to_sym) + end + def internal_store if @store_opened_by_pid != process_id @store_opened_by_pid = process_id diff --git a/spec/prometheus/client/data_stores/direct_file_store_spec.rb b/spec/prometheus/client/data_stores/direct_file_store_spec.rb index c140546e..14e12bf2 100644 --- a/spec/prometheus/client/data_stores/direct_file_store_spec.rb +++ b/spec/prometheus/client/data_stores/direct_file_store_spec.rb @@ -115,6 +115,46 @@ ) end + it "decodes each label set once per export, however many processes wrote it" do + [12345, 23456, 34567].each do |pid| + allow(Process).to receive(:pid).and_return(pid) + metric_store.increment(labels: { foo: "bar" }, by: 1) + metric_store.increment(labels: { foo: "baz" }, by: 1) + end + + expect(metric_store).to receive(:parse_labelset).twice.and_call_original + expect(metric_store.all_values).to eq( + { foo: "bar" } => 3.0, + { foo: "baz" } => 3.0, + ) + end + + it "encodes each label set once, however many times it's written" do + expect(metric_store).to receive(:encode_labelset).twice.and_call_original + + 3.times do + metric_store.increment(labels: { foo: "bar" }, by: 1) + metric_store.increment(labels: { foo: "baz" }, by: 1) + end + + expect(metric_store.all_values).to eq( + { foo: "bar" } => 3.0, + { foo: "baz" } => 3.0, + ) + end + + it "isn't affected by callers mutating a label set after writing it" do + labels = { foo: "bar" } + metric_store.increment(labels: labels, by: 1) + labels[:foo] = "baz" + metric_store.increment(labels: labels, by: 2) + + expect(metric_store.all_values).to eq( + { foo: "bar" } => 1.0, + { foo: "baz" } => 2.0, + ) + end + context "for a non-gauge metric" do it "sums values from different processes by default" do allow(Process).to receive(:pid).and_return(12345)