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.

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

Document metrics definition behavior with fork #212

Merged
merged 1 commit into from
Mar 20, 2021

Conversation

papa-cool
Copy link
Contributor

@papa-cool papa-cool commented Jan 11, 2021

I've followed the documentation in order to setup prometheus client with Rails.
The use of ::Prometheus::Client::DataStores::DirectFileStore was very clear. However, it was unexpected for me to have the list of the metrics stored in memory.
When testing locally, I've got empty result from /metrics because the metrics were defined in another process that the endpoint.
Definition of metrics after fork can lead to random inconsistencies. That's why I propose to alert on this behavior.

Signed-off-by: Emmanuel Delmas emmanuel.delmas@jobteaser.com

@coveralls
Copy link

coveralls commented Jan 11, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling bd38317 on papa-cool:metrics_before_fork into 1c8e407 on prometheus:master.

Signed-off-by: Emmanuel Delmas <emmanuel.delmas@jobteaser.com>
@Sinjo
Copy link
Member

Sinjo commented Mar 20, 2021

Hi @papa-cool, and thank you for the contribution!

You're right, there are some unintuitive consequences of using the DirectFileStore and this is another one worth documenting.

I just had a read through the relevant code and a chat with @dmagliola to refresh my mind on why it works the way it does. In theory, it would be nice for us to go straight to the metric storage when we want to serialise all the metrics for export. There are a couple of issues with that approach though:

  • Right now the exporter doesn't have to care about metric storage, as that's abstracted behind the metric interface. This isn't a decision we're unwilling to change in future (see the point below about performance), but it's a tradeoff we need to have a think through.
  • Metrics can be part of different registries, and it's only the registry objects that know about that. The storage layer doesn't currently concern itself with them, so we'd have to rework some of the method parameters in the storage interface and the serialisation of DirectFileStore itself (and we'd be losing the abstraction mentioned above).

Going straight to the storage is something we'd talked about before for another reason: export performance. Right now the exporter has to go through the registry, which then iterates through each metric within it, and asks the storage for the values of each metric. Naturally, this is slower than being able to say "hey storage, please serialise the current state of everything". The downside of that approach is that the storage and exporter layers become tightly coupled.

Possibly a longer answer than you were expecting, but hopefully the context makes sense (and selfishly, I wanted to get this written down somewhere rather than trying to remember it again next time).

We are going to revisit this design, but it's not going to be an easy/obvious one for us to solve, so in the meantime your contribution to the docs is absolutely the right thing to do. Thank you again!

@Sinjo Sinjo merged commit f4c9e08 into prometheus:master Mar 20, 2021
@papa-cool papa-cool deleted the metrics_before_fork branch March 22, 2021 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants