8000 Fix bug of multiprocess mode by `path` argument by Sparkycz · Pull Request #395 · prometheus/client_python · GitHub
[go: up one dir, main page]
Skip to content

Fix bug of multiprocess mode by path argument #395

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
Closed
wants to merge 2 commits into from
Closed

Fix bug of multiprocess mode by path argument #395

wants to merge 2 commits into from

Conversation

Sparkycz
Copy link
@Sparkycz Sparkycz commented Apr 9, 2019 •
edited
Loading

path argument at multiprocess.MultiProcessCollector.__init__() is only for merging multiprocess files but it doesn't work for processing metrics and creating that files (without set os.environ.['prometheus_multiproc_dir']). Everybody has to activate the mode by the environment variable prometheus_multiproc_dir.

works:

import os
from prometheus_client import REGISTRY

os.environ.['prometheus_multiproc_dir'] = '/tmp'

MultiProcessCollector(REGISTRY)

doesn't work:

from prometheus_client import REGISTRY

MultiProcessCollector(REGISTRY, path='/tmp')

@Sparkycz
Fix bug of multiprocess mode by path argument
ec84762 
Signed-off-by: Vladimír Kašpar <vladimir.kaspar@heureka.cz>
@Sparkycz Sparkycz force-pushed the multiprocess_by_path_arg branch from 117d591 to ec84762 Compare April 9, 2019 13:35
@brian-brazil
Copy link
Contributor

How exactly isn't the latter working?

@Sparkycz
Copy link
Author
Sparkycz commented Apr 9, 2019

before my change - If you don't set the environment variable the library doesn't create that .db files because at values it consumes only the environemnt variable and doesn't consume the path set at MultiProcessCollector constructor. It means everybody must set the environment variable.

after my change - if you set the path by path argument at MultiProcessCollector constructor it sets the global variable PATH which is consumed everywhere it's needed

@brian-brazil
Copy link
Contributor
< 8000 /tr>

This is the expected behaviour, you must set the environment variable for the multi processing to work.

There's however no need to set it for MultiProcessCollector to work.

@Sparkycz
Copy link
Author
Sparkycz commented Apr 9, 2019

Why is there the path argument?

@Sparkycz
Copy link
Author
Sparkycz commented Apr 9, 2019

My change doesn't block to use the environment variable..

@brian-brazil
Copy link
Contributor

In case you want to point it at a different directory for some reason. Your change doesn't make sense overall I'm afraid, the path for collectors needs to be configured before the process starts.

@Sparkycz
Copy link
Author
Sparkycz commented Apr 9, 2019 •
edited
Loading

Ok, I got it..

However, the use of an environment variable seems unhappy to me in this case (as a flag for library)..

Do you think of another solution how to change it to active multiprocess mode without ENV variable?

@Sparkycz
Copy link
Author
Sparkycz commented Apr 9, 2019 •
edited
Loading

I try another change..

It sets the global variable during the process starts from the environment variable or it's overwritten by the path argument.
you're still able to expose metrics from another dictionary because the function prometheus_generate_latest() has the argument for injecting the collector you want to expose metrics from.

@Sparkycz
some change that will be merged..
3c79a56 
Signed-off-by: Vladimír Kašpar <vladimir.kaspar@heureka.cz>
@Sparkycz Sparkycz force-pushed the multiprocess_by_path_arg branch from bc4e547 to 3c79a56 Compare April 9, 2019 14:20
@djetelina
Copy link

The whole root of this pull request is our misunderstanding of the path argument in MultiprocessCollector. We assumed that it was an alternative to the environment variable - which I'd guess can be a common mistake for people that skip over the sentence The prometheus_multiproc_dir environment variable must be set in README and start experimenting right away instead. My humble suggestion would be to remove the argument, as it gives false 'hope' that there is an alternative...

Then there's the attempt to 'improve' this and make the environmental variable an alternative rather than a requirement. After @Sparkycz sent me the link to this PR we went together through the code and realized that due to the singleton nature of this library it's quite clearly not worth it to implement that and any attempt would just break the current behavior that some might rely on already.

@Sparkycz Sparkycz closed this Apr 9, 2019
@iostream96 iostream96 mentioned this pull request Feb 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sparkycz @brian-brazil @djetelina
0