Skip to content

Commit

Permalink
Fix the "subtle" bug introduced in the previous commit
Browse files Browse the repository at this point in the history
So, again, I don't propose we actually do this, this is just illustrative
of what we need to do, and didn't want it to get lost.

The problem with the previous commit is that for each metric, we open a
new FileMappedDict. All of them sharing the same file is "fine", actually.
The problem comes from the fact that each instance of FileMappedDict has
their own version of `@used`. When one metric adds a new entry to the file,
it moves its own `@used` pointed, but the other instances don't notice this.

As soon as a second instance that had already been loaded adds a new labelset,
it corrupts the file.

This only shows up when running the benchmarks, with histograms. Counters
(in our benchmark) don't reproduce it, because each metric in our benchmark
only has one labelset. They all get added initially to the file, and an
already loded Dict doesn't add new ones. The reason Histograms reproduce it
is that new observations may report a bucket that wasn't seen before, which
is effectively a new labelset.

The solution is for all MetricStore instances to share one single FileMappedDict.
This is not particularly hard, but the current MetricStore is not written
in a way that allows that without doing this crap.

We should find a better way. I'm just leaving this here as documentation
of this problem, for the next brave soul attempting this
  • Loading branch information
Daniel Magliola committed Oct 14, 2019
1 parent d8b247b commit 149fc3c
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 0 deletions.
22 changes: 22 additions & 0 deletions lib/prometheus/client/data_stores/direct_file_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ def validate_metric_settings(metric_settings)
class MetricStore
attr_reader :metric_name, :store_settings

class << self
attr_accessor :shared_store_opened_by_pid
attr_accessor :shared_internal_store
end

def initialize(metric_name:, store_settings:, metric_settings:)
@metric_name = metric_name
@store_settings = store_settings
Expand Down Expand Up @@ -164,6 +169,14 @@ def store_key(labels)
end

def internal_store
if @store_settings[:separate_files_per_metric]
individual_metric_internal_store
else
all_metrics_shared_internal_store
end
end

def individual_metric_internal_store
if @store_opened_by_pid != process_id
@store_opened_by_pid = process_id
@internal_store = FileMappedDict.new(filemap_filename)
Expand All @@ -172,6 +185,15 @@ def internal_store
end
end

def all_metrics_shared_internal_store
if self.class.shared_store_opened_by_pid != process_id
self.class.shared_store_opened_by_pid = process_id
self.class.shared_internal_store = FileMappedDict.new(filemap_filename)
else
self.class.shared_internal_store
end
end

# Filename for this metric's PStore (one per process)
def filemap_filename
filename = if @store_settings[:separate_files_per_metric]
Expand Down
2 changes: 2 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 @@ -10,6 +10,8 @@
# Reset the PStores
before do
Dir.glob('/tmp/prometheus_test/*').each { |file| File.delete(file) }
# This doesn't actually work, btw, but it's what would have to be done.
Prometheus::Client::DataStores::DirectFileStore::MetricStore.shared_store_opened_by_pid = nil
end

it_behaves_like Prometheus::Client::DataStores
Expand Down

0 comments on commit 149fc3c

Please sign in to comment.