From ac35eb9b273bcb4ce442a1c6f3ca463347928a0f Mon Sep 17 00:00:00 2001 From: Maximilian Bernwieser Date: Tue, 27 Oct 2026 14:59:44 +0100 Subject: [PATCH 1/2] Decode each DirectFileStore label set once per export `MetricStore#all_values` decoded the key of every entry in every process's file. Since each process's file usually holds the same keys, the same label set got decoded once per process on every export. Since `URI.decode_www_form` replaced `CGI::parse` in 5.0.0, decoding is the bulk of export time for long label values, so cache decoded label sets for the duration of the call. Signed-off-by: Maximilian Bernwieser --- .../client/data_stores/direct_file_store.rb | 18 +++++++++++++----- .../data_stores/direct_file_store_spec.rb | 14 ++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/lib/prometheus/client/data_stores/direct_file_store.rb b/lib/prometheus/client/data_stores/direct_file_store.rb index 05a506c6..0253ff13 100644 --- a/lib/prometheus/client/data_stores/direct_file_store.rb +++ b/lib/prometheus/client/data_stores/direct_file_store.rb @@ -127,17 +127,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 @@ -165,6 +167,12 @@ def store_key(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..11d41282 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,20 @@ ) 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 + context "for a non-gauge metric" do it "sums values from different processes by default" do allow(Process).to receive(:pid).and_return(12345) From 9fd1f786ec52954c92e51fc72abe6f35729ca397 Mon Sep 17 00:00:00 2001 From: Maximilian Bernwieser Date: Tue, 27 Oct 2026 15:00:20 +0100 Subject: [PATCH 2/2] Cache encoded DirectFileStore keys per label set `store_key` encoded the label set on every `set`, `increment` and `get`. Since `URI.encode_www_form` replaced `CGI::escape` in 5.0.0, that is several times slower for long label values, and a histogram `observe` pays it twice. Remember each label set's key instead. The cache is keyed on a frozen copy of the labels, since callers may mutate their hash afterwards, and it's guarded by the store's existing lock, since `increment` calls `store_key` outside it. `:all` aggregation isn't cached, because its key includes the current pid. The cache grows by one entry per distinct label set, which is the same bound as the store's file and its in-memory `positions` index. Signed-off-by: Maximilian Bernwieser --- .../client/data_stores/direct_file_store.rb | 10 +++++++ .../data_stores/direct_file_store_spec.rb | 26 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/lib/prometheus/client/data_stores/direct_file_store.rb b/lib/prometheus/client/data_stores/direct_file_store.rb index 0253ff13..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 @@ -162,8 +163,17 @@ 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 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 11d41282..14e12bf2 100644 --- a/spec/prometheus/client/data_stores/direct_file_store_spec.rb +++ b/spec/prometheus/client/data_stores/direct_file_store_spec.rb @@ -129,6 +129,32 @@ ) 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)