Multiprocess environment variable in upper case #624

marcaurele · 2021-02-09T05:47:18Z

I would like to propose a change to move the prometheus_multiproc_dir env variable in upper case to follow the convention of such variable. It feels strange to have to declare it in lower case.

The code adds a deprecation warning to inform the user about the change and transform it to upper case if needed. It also take care of precedence in case both are declared.

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele · 2021-02-11T10:26:33Z

cc @csmarchbanks

csmarchbanks

Thank you for your contribution!

I think accepting an upper case environment variable certainly makes sense. However, I am not sure we need to deprecate the lower case version though as it is still valid. I don't have a windows machine to test on right now, but is there any concern about case insensitivity?

README.md

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

Following PR prometheus#624 Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks · 2021-02-16T17:52:52Z

prometheus_client/values.py

-    if 'prometheus_multiproc_dir' in os.environ:
+    if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' not in os.environ:
+        os.environ['PROMETHEUS_MULTIPROC_DIR'] = os.environ['prometheus_multiproc_dir']
+        warnings.warn("You should declare the env variable in upper case PROMETHEUS_MULTIPROC_DIR'", DeprecationWarning)


Looks like I left a comment about the warnings in the overall comment instead of a specific one.

My inclination is to allow either lower or upper case without any deprecation warnings as both are valid. Do you have any concerns with just allowing either?

I would like to encourage people to use the upper case to follow standard practice in Unix env variables. If we don't put that in the readme, we don't say anything in the code to promote the use of the uppercase I don't see how people will move to that variable name in upper case.

That seems reasonable, let's keep the deprecation notice. I do plan to merge your README change after I do a release with this change as well.

csmarchbanks

Thanks for the reasoning on the deprecation notice. I did a bit of testing and found one concern.

prometheus_client/values.py

csmarchbanks · 2021-02-19T16:26:54Z

prometheus_client/values.py

-    if 'prometheus_multiproc_dir' in os.environ:
+    if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' not in os.environ:
+        os.environ['PROMETHEUS_MULTIPROC_DIR'] = os.environ['prometheus_multiproc_dir']
+        warnings.warn("You should declare the env variable in upper case PROMETHEUS_MULTIPROC_DIR'", DeprecationWarning)


That seems reasonable, let's keep the deprecation notice. I do plan to merge your README change after I do a release with this change as well.

In case people use directly `MultiProcessValue`, the deprecation warning will pop up. Added another warning if both are defined. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

@csmarchbanks

thanks to @csmarchbanks Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks

Nice work, I noticed a couple extra characters in the warnings, and came across one edge case in testing. Curious on your thoughts there, if it would be too complex I think the current behavior is fine.

prometheus_client/values.py

csmarchbanks · 2021-03-02T15:13:28Z

prometheus_client/values.py

        def __init__(self, typ, metric_name, name, labelnames, labelvalues, multiprocess_mode='', **kwargs):
            self._params = typ, metric_name, name, labelnames, labelvalues, multiprocess_mode
+            # This deprecation warning can go away in a few releases when removing the compatibility
+            if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' in os.environ:


If I instantiate multiple metrics, I end up getting both warnings because this clause is triggered on the second metric after the capital environment variable is set. Is there a good way to not get two warnings in that case? Here is a quick example:

from prometheus_client import generate_latest, CollectorRegistry, multiprocess, Gauge registry = CollectorRegistry() multiprocess.MultiProcessCollector(registry) Gauge('metric_1', 'First metric', registry=registry) Gauge('metric_2', 'Second metric', registry=registry)

Since they are just warnings, not a big deal, but it is a bit confusing.

One option would be to always warn if the lowercase is set, then only replace if the capitalized version is not set? Users with both set would only get the You should declare the env variable in upper case PROMETHEUS_MULTIPROC_DIR in that case.

Yes, correct. I'd rather display the warning for the ones whom only defined the variable in lowercase. I'll the corresponding lines.

👍 Sounds good, let me know once that is complete and I will test again.

Have a look. I also change the message on the deprecation warning as it did not sound so much as one. I think the wording is much better now. Don't hesitate if you have any other proposal to do.

prometheus_client/values.py

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

It does not trigger a deprecation warning anymore. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

Remove the warning when both are used and rephrase the message to be more deprecation aware. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks

The new messaging looks good to me. I don't know how I missed it in my previous tests but there are a few places in prometheus_client/multiprocess.py where prometheus_multiproc_dir is still used. Specifically, when creating a MultiProcessCollector, and during mark_process_dead. Those will need to be updated too. Sorry about missing that earlier and needing extra revisions!

It looks like the tests are passing only because self.tempdir is passed in as a path rather than the default of None. It might be worth having a test case for the None scenario as well.

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele · 2021-03-29T05:09:35Z

@csmarchbanks you can do your tests again. Let me know if there's anything else that needs to be changed.

csmarchbanks

Awesome, I ran through my tests again and this is looking good. Thanks for all the work!

Closes #436. Related to prometheus/client_python#624.

marcaurele added 2 commits February 9, 2021 06:47

Use upper case environment variable for multi process

2d2fc94

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

add test cases for deprecation warning on variable capitalization

156a5e8

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele force-pushed the env-var-upper-case branch from 9b5a49f to 156a5e8 Compare February 9, 2021 05:48

marcaurele added 3 commits February 9, 2021 06:52

fix isort

3ab5a3c

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

fix isort

c451b2e

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

nitpick: unused variable

9bcbb68

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele force-pushed the env-var-upper-case branch from 28986cc to 9bcbb68 Compare February 9, 2021 05:58

csmarchbanks reviewed Feb 11, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

readme: keep lower case

38c65cf

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele force-pushed the env-var-upper-case branch from 2614f83 to 38c65cf Compare February 14, 2021 11:15

marcaurele added a commit to marcaurele/client_python that referenced this pull request Feb 14, 2021

readme: inform on the upper case variable

9b7fcc2

Following PR prometheus#624 Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele mentioned this pull request Feb 14, 2021

readme: inform on the upper case variable #628

Merged

csmarchbanks reviewed Feb 16, 2021

View reviewed changes

csmarchbanks reviewed Feb 19, 2021

View reviewed changes

move the deprecation warning inside MultiProcessValue

2ba399d

In case people use directly `MultiProcessValue`, the deprecation warning will pop up. Added another warning if both are defined. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele force-pushed the env-var-upper-case branch from 4a6b656 to 2ba399d Compare February 22, 2021 16:34

fix test

e448d98

thanks to @csmarchbanks Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks reviewed Mar 2, 2021

View reviewed changes

marcaurele added 3 commits March 2, 2021 17:16

🦅 catch typo

8028b6f

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

test: remove test with both variables

63e6cd5

It does not trigger a deprecation warning anymore. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

Change warning deprecation on env variable

48c4fb6

Remove the warning when both are used and rephrase the message to be more deprecation aware. Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks reviewed Mar 2, 2021

View reviewed changes

marcaurele added 2 commits March 29, 2021 06:45

test: change tests to make them fail

02209e0

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

partial fix

cf2d434

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

marcaurele force-pushed the env-var-upper-case branch from 60694fa to cf2d434 Compare March 29, 2021 04:45

env var: capitalize it and display warning if needed

64e584d

Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>

csmarchbanks approved these changes Apr 2, 2021

View reviewed changes

csmarchbanks merged commit f22d38f into prometheus:master Apr 2, 2021

qbiqing mentioned this pull request Apr 8, 2021

prometheus_multiproc_dir env var to switch to upper case rycus86/prometheus_flask_exporter#97

Closed

sm5111 mentioned this pull request Oct 18, 2021

Please update deprecated prometheus_multiproc_dir environment variable Bogdanp/dramatiq#436

Closed

Bogdanp added a commit to Bogdanp/dramatiq that referenced this pull request Oct 23, 2021

prometheus: set both upper and lower case multiproc env var

51f5c92

Closes #436. Related to prometheus/client_python#624.

neilferreira mentioned this pull request Dec 8, 2021

Project still uses prometheus_multiproc_dir environment variable in lower-case trallnag/prometheus-fastapi-instrumentator#89

Closed

michael-angelo-guban pushed a commit to michael-angelo-guban/python-dramatic that referenced this pull request Apr 13, 2025

prometheus: set both upper and lower case multiproc env var

223e3af

Closes #436. Related to prometheus/client_python#624.

Multiprocess environment variable in upper case #624

Multiprocess environment variable in upper case #624

Uh oh!

Conversation

marcaurele commented Feb 9, 2021

Uh oh!

marcaurele commented Feb 11, 2021

Uh oh!

csmarchbanks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csmarchbanks Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

marcaurele Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

csmarchbanks Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csmarchbanks Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csmarchbanks Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

marcaurele Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csmarchbanks Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

marcaurele Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csmarchbanks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaurele commented Mar 29, 2021

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

csmarchbanks left a comment •

edited

Loading

marcaurele Mar 2, 2021 •

edited

Loading

csmarchbanks left a comment •

edited

Loading