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

Add multiprocess support #16

Merged
merged 9 commits into from
Oct 24, 2024

Conversation

VladVolchkov
Copy link
Contributor

@VladVolchkov VladVolchkov commented Feb 14, 2024

relates to #15

@deepminimal
Copy link

deepminimal commented Mar 22, 2024

@savar @mbarouski Hello! Can you check it please?

@savar
Copy link
Collaborator

savar commented Apr 6, 2024

@savar @mbarouski Hello! Cat you check it please?

Hey, I am not experienced in this multiprocess part of the prometheus client.
I am trying to read the doc but I am not sure that this Collector stuff can be done only once in the initializer as the example in https://prometheus.github.io/client_python/multiprocess/ is doing this every time the scrape is happening.

Also as far as I can tell the multiprocess would require a properly set PROMETHEUS_MULTIPROC_DIR environment variable to work. Maybe we need to test for this envrionment variable to be set and only if, then we start the multiprocess part, otherwise we stick to the existing setup?

Do you have a working example deployment where this is being used, so that we could have a better look onto it?

@savar
Copy link
Collaborator

savar commented Apr 6, 2024

It looks like it could work but we will lose the python_* and process_* as well as the *_created metrics. Also I would like to not use this when there is only a single process being used, specifically because we do not execute the CollectorRegistry+MultiProcessCollector on each scrape request but only once initially. The documentation states it has a risk, but I cannot say right now if this applies to us here.

I just cannot figure out how we could detect the multiprocess situation. Thumbor is not providing this information in the config it passes into the initializer and relying on the environment variable would only mean that people do not realize that they didn't configure it correctly. 🤔

@sboisson
Copy link

sboisson commented Jul 5, 2024

Maybe only use CollectorRegistry and MultiProcessCollector when PROMETHEUS_MULTIPROC_DIR env var is set ?

Copy link
Collaborator

@savar savar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try it like that. I am still not 100% sure that it always works perfectly but it looks like it can. But without the environment variable it won't work, so not doing it without it.

I will change some more stuff and add some documentation when this is merged.

tc_prometheus/metrics/prometheus_metrics.py Show resolved Hide resolved
tc_prometheus/metrics/prometheus_metrics.py Outdated Show resolved Hide resolved
VladVolchkov and others added 2 commits October 23, 2024 14:09
Co-authored-by: Simon Effenberg <savar@schuldeigen.de>
Co-authored-by: Simon Effenberg <savar@schuldeigen.de>
@savar savar merged commit 234db20 into thumbor-community:master Oct 24, 2024
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.

5 participants