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

Issue with prometheus_multiproc_dir deprecation #643

Closed
tonial opened this issue Apr 6, 2021 · 4 comments · Fixed by #644
Closed

Issue with prometheus_multiproc_dir deprecation #643

tonial opened this issue Apr 6, 2021 · 4 comments · Fixed by #644

Comments

@tonial
Copy link

tonial commented Apr 6, 2021

Hi !

I noticed an issue with prometheus_multiproc_dir deprecation in the latest 0.10.0 version.
In mark_process_dead() the environment variable transition code is not present, which means that if the uppercase version is not set, the function does not work.
https://github.com/prometheus/client_python/pull/624/files#diff-95ab27209a655ee3a508d436368ba4e1d155784c5885e18be09b61c2a5ec03efL155

Since we can call mark_process_dead() without going through the __init__() methods that set the new environment variable, it would required something like this : https://github.com/prometheus/client_python/pull/624/files#diff-f1ab5b765643c2beafab5bcd9874a839f7ebf080300e1ee0dbdb821755f8e904R56-R58

@csmarchbanks
Copy link
Member

Hello, good catch! I see how that could be an issue if someone marks a process as dead without ever setting up a collector. Would you be interested in opening a PR? Otherwise I will fix it and release v0.10.1.

@romuald
Copy link
Contributor

romuald commented Apr 8, 2021

@csmarchbanks note that the README was not updated by the commit. I assume this is an oversight
(edit: also I realize I'm not in the correct issue .. sorry)

@csmarchbanks
Copy link
Member

Since everything was supposed to work with either version I left the README alone for a bit until people had time to upgrade and not be confused if they were still running 0.9.0. There is a second PR (#628) to update the README that I plan to merge soon.

@tonial
Copy link
Author

tonial commented Apr 13, 2021

Oh ! I'm sorry @csmarchbanks, I should have made a PR right away when creating the issue.
I got sidetracked with other issues and forgot to come back and check your answer.

Thanks a lot for your very short reaction time to fix the issue :)

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 a pull request may close this issue.

3 participants