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

Deprecate lower-case 'prometheus_multiproc_dir' #42

Closed
wants to merge 1 commit into from
Closed

Deprecate lower-case 'prometheus_multiproc_dir' #42

wants to merge 1 commit into from

Conversation

michaelusner
Copy link
Contributor

The Prometheus Python client has deprecated the lower-case prometheus_multiproc_dir environment variable.
This PR brings the check and warning for this condition into parity with https://github.com/prometheus/client_python/blob/9a6e21de144e81ac666828aa843efb398d184b27/prometheus_client/multiprocess.py#L28

The Prometheus Python client has deprecated the lower-case ```prometheus_multiproc_dir``` environment variable.
This PR brings the check and warning for this condition into parity with https://github.com/prometheus/client_python/blob/9a6e21de144e81ac666828aa843efb398d184b27/prometheus_client/multiprocess.py#L28
Copy link

@mattmp-mercari mattmp-mercari left a comment

Choose a reason for hiding this comment

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

Thanks for this! I was about to write the same PR but thankfully found yours.

The changes look good to me. This fixes #89 and #50.

Our team ran into a metrics scraping problem like in #50 where multiprocess mode was not enabled due to prometheus-fastapi-instrumentator not finding for PROMETHEUS_MULTIPROC_DIR (uppercase). It took a non-trivial amount of time to get to the root cause.

@michaelusner
Copy link
Contributor Author

michaelusner commented May 25, 2022 via email

@tomtom103
Copy link
Contributor

+1 For this, caused our team some major headaches...

@michaelusner
Copy link
Contributor Author

@mattmp-mercari Is there anything else I need to do to get some eyes on this PR? It's been in limbo for more than a year now. Please let me know if there's anything I need to do from my end. Thanks!

@trallnag
Copy link
Owner

Closing because I have merged this in #217. Failed to push the rebase to this branch / PR.

@trallnag
Copy link
Owner

Thank you, @michaelusner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants