Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

One file per process instead of per metric/process for DirectFileStore [DOESNT WORK] #161

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

One file per process instead of per metric/process for DirectFileStore [DOESNT WORK] #161

wants to merge 2 commits into from

Commits on Oct 14, 2019

  1. WIP: Allow having all metrics in one file, #143 

    This was a quick experiment on having all metrics for each process on
the same file, because it seemed like it would be easy. (just do these
things in this commit)

However, even though tests pass, this doesn't actually work. See next commit on why.
If the change were just this, i'd say we should do this.

However, as you'll see in the next commit, it's more involved than that,
and i'm not sure it's worth doing, at least not with this approach...

I'm just pushing this up so it doesn't get lost.
    Daniel Magliola committed Oct 14, 2019
    Configuration menu
    Copy the full SHA
    d8b247b View commit details
    Browse the repository at this point in the history

  2. Fix the "subtle" bug introduced in the previous commit 

    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
    Daniel Magliola committed Oct 14, 2019
    Configuration menu
    Copy the full SHA
    149fc3c View commit details
    Browse the repository at this point in the history