Constance metrics #73

msalaban-reef · 2022-06-08T20:39:40Z

This PR allows for exporting django-constance config variables as Prometheus metrics.

It supports int, float, bool and decimal.Decimal types by default.
It skips variables listed in CONSTANCE_PROMETHEUS_BLACKLIST setting.
It processes variables listed in CONSTANCE_PROMETHEUS_WHITELIST setting by adding their string representation to the metric's name. (May cause problems in multiprocess mode.)

This is a review request, esp. to @ppolewicz

Variables from CONSTANCE_CONFIG will be exported as Prometheus metrics, starting with the default prefix of "constance_config_"

Not all string characters are allowed in metric names. Try to replace them with underscores.

The Enum type is being dropped in multiprocess mode and Gauge is perfectly enough to represent booleans

lgtm-com · 2022-06-08T20:45:44Z

This pull request introduces 3 alerts and fixes 1 when merging e8aa7a7 into c85cafe - view on LGTM.com

new alerts:

2 for Syntax error
1 for Unused import

fixed alerts:

1 for Syntax error

lgtm-com · 2022-06-08T20:53:05Z

This pull request introduces 2 alerts and fixes 1 when merging 95da931 into c85cafe - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

cookiecutter.json

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

ppolewicz · 2022-06-11T19:39:08Z

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

+            n = f"{n}_{v}"
+        return n
+
+    def _unregister(self):


@mpnowacki-reef I think you might know what to do here. I mean for me it's pretty clear that unregistering should be implemented in metrics.py and not in constance_prometheus.py, registry seems to be a singleton so ideally we'd see some CollectorRegistryProvider which would just return the exiting object, rather than the class magically returning the same object as created somewhere else or the class somehow working consistently across threads despite having multiple copies...

But other than that I don't know how to deal with the multiprocess unregistering.

registry seems to not be a singleton, according to prometheus-client howto: https://github.com/prometheus/client_python#multiprocess-mode-eg-gunicorn

I'm a bit lost, it is my understanding that registry.unregister is for unregistering Collectors, not individual metrics, what ma I missing?

Nevermind that ^

My question is: does it actually work? When collecting metrics in multiprocessing mode, their values are read from .db files. I don't se how unregister would remove entries from those files but I can be wrong.

Frankly speaking, from what I see, removing metrics does not really work when in multiprocessing mode. But again, I could be wrong. This whole thing is a little bit spaghetti in prometheus-client. Removing labels, however, should work, so a single metric with labels for constance param name and value could be a quasi-supported solution. And it would, I think, solve the duplicates problem

I think it's kind of fixed right now. I say kind because 15cd1fc is nothing but a hack. It seems to work in my local test project but will likely bite our collective ass in the future. What do you think?

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

ppolewicz · 2022-06-11T19:42:41Z

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

+    def _store_str(self, value):
+        value = str(value)


make the caller pass the correct type to the function

The caller would have to do another type check then, which would make the code unnecessarily complex.
ATM the only additional types allowed by constance are those from datetime and str works on them pretty well.

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

lgtm-com · 2022-06-14T20:26:45Z

This pull request introduces 2 alerts and fixes 1 when merging a1b416d into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

As ideated in the CR, there could be two functions: - one which stores a single monitored variable, - another which stores all of them. This commit implements them.

lgtm-com · 2022-06-14T20:40:52Z

This pull request introduces 2 alerts and fixes 1 when merging c02cd1c into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

msalaban-reef · 2022-06-14T20:48:41Z

I updated the code after your review. Please re-check. The important outstanding issue is unregistering metrics in multiprocess environment, however for that I'd like to consult someone with practical experience.

lgtm-com · 2022-06-14T20:53:11Z

This pull request introduces 2 alerts and fixes 1 when merging 8f48671 into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

msalaban-reef · 2022-06-15T07:11:31Z

Another idea here is to keep the following types permitted by constance as:

datetime: .totimestamp(), .fromtimestamp()
date: .toordinal(), fromordinal()
timedelta: td / timedelta(microseconds=1)
time: in microseconds, but the conversion methods aren't provided explicitly

The advantage is that we could keep them as floats, at the cost of additional conversion on both ends. Problems arise, however, when timezones are used and need to be conserved (otherwise we could normalize to UTC).

@ppolewicz what do you think about it?

ppolewicz · 2022-06-15T15:02:18Z

Always use UTC time.

I'd rather cast it to int and have a 1s precision.

mpnowacki-reef · 2022-06-16T11:16:13Z

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py

+            # a str or custom type; cast everything to str
+            value = str(value)
+            self.name = self._get_name(value)
+            self._metric = Gauge(self.name, self.description)


I'm worried about the multiprocess_mode being let default here, which is all. I think what we need is livemax. The current behaviour will be:

'all': Default. Return a timeseries per process (alive or dead), labelled by the process's pid (the label is added internally).

If constance yields different values in different processes, it's either a misconfiguration or a bug, not sure we should be detecting it here

It doesn't help there, either. Adding the mode, I still get a duplicate error when changing the variable back to previously used value:

Environment: Request Method: POST Request URL: http://localhost:8000/admin/constance/config/ Django Version: 3.2.12 Python Version: 3.10.4 Installed Applications: ['django_prometheus', 'django.contrib.admin', 'django.contrib.auth', 'django.contrib.contenttypes', 'django.contrib.sessions', 'django.contrib.messages', 'django.contrib.staticfiles', 'django_extensions', 'django_probes', 'constance', 'project.core', 'corsheaders'] Installed Middleware: ['corsheaders.middleware.CorsMiddleware', 'django_prometheus.middleware.PrometheusBeforeMiddleware', 'django.middleware.security.SecurityMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.middleware.common.CommonMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', 'django_prometheus.middleware.PrometheusAfterMiddleware'] Traceback (most recent call last): File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/core/handlers/exception.py", line 47, in inner response = get_response(request) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/core/handlers/base.py", line 181, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/utils/decorators.py", line 130, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/views/decorators/cache.py", line 44, in _wrapped_view_func response = view_func(request, *args, **kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/contrib/admin/sites.py", line 232, in inner return view(request, *args, **kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/utils/decorators.py", line 43, in _wrapper return bound_method(*args, **kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/utils/decorators.py", line 130, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/emes/venvs/reef/lib/python3.10/site-packages/constance/admin.py", line 256, in changelist_view form.save() File "/home/emes/venvs/reef/lib/python3.10/site-packages/constance/admin.py", line 157, in save setattr(config, name, new) File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/utils/functional.py", line 277, in __setattr__ setattr(self._wrapped, name, value) File "/home/emes/venvs/reef/lib/python3.10/site-packages/constance/base.py", line 29, in __setattr__ self._backend.set(key, value) File "/home/emes/venvs/reef/lib/python3.10/site-packages/constance/backends/redisd.py", line 50, in set signals.config_updated.send( File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 180, in send return [ File "/home/emes/venvs/reef/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 181, in <listcomp> (receiver, receiver(signal=self, sender=sender, **named)) File "/home/emes/devel/reef/cct/project/app/src/project/core/constance_prometheus.py", line 134, in on_constance_config_updated store_monitored_metric(key, new_value) File "/home/emes/devel/reef/cct/project/app/src/project/core/constance_prometheus.py", line 117, in store_monitored_metric METRICS[key].store(new_value) File "/home/emes/devel/reef/cct/project/app/src/project/core/constance_prometheus.py", line 106, in _store_str self._metric = Gauge(self.name, self.description) File "/home/emes/venvs/reef/lib/python3.10/site-packages/prometheus_client/metrics.py", line 365, in __init__ super().__init__( File "/home/emes/venvs/reef/lib/python3.10/site-packages/prometheus_client/metrics.py", line 143, in __init__ registry.register(self) File "/home/emes/venvs/reef/lib/python3.10/site-packages/prometheus_client/registry.py", line 43, in register raise ValueError( Exception Type: ValueError at /admin/constance/config/ Exception Value: Duplicated timeseries in CollectorRegistry: {'constance_config_SOME_STRING_hello_string'}

I wonder if we shouldn't drop the str (and other non-standard type) monitoring until we figure out a way to handle it better.

Nevermind the above comment. I'll address the "livemax" mode later on.
BTW, have you noticed that "livemax" is available only in unreleased code? Latest official release (0.14.1) doesn't have it.

lgtm-com · 2022-06-19T20:53:26Z

This pull request introduces 3 alerts and fixes 1 when merging 016937f into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error
1 for Unused import

fixed alerts:

1 for Syntax error

lgtm-com · 2022-06-19T21:14:45Z

This pull request introduces 2 alerts and fixes 1 when merging 740fd69 into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

THIS IS A HACK: To avoid key clash, try to reuse an existing metric by using undocumented mapping in the registry.

lgtm-com · 2022-06-20T18:37:39Z

This pull request introduces 3 alerts and fixes 1 when merging 15cd1fc into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error
1 for Unused import

fixed alerts:

1 for Syntax error

lgtm-com · 2022-06-20T18:47:07Z

This pull request introduces 2 alerts and fixes 1 when merging 38690b1 into b54e5b6 - view on LGTM.com

new alerts:

2 for Syntax error

fixed alerts:

1 for Syntax error

msalaban-reef · 2022-07-14T07:05:13Z

Any progress with reviewing this, guys?

msalaban-reef added 5 commits June 8, 2022 22:29

Add constance metrics export

e857a0f

Variables from CONSTANCE_CONFIG will be exported as Prometheus metrics, starting with the default prefix of "constance_config_"

Replace non-word chars with underscores in strings

7cbfb0f

Not all string characters are allowed in metric names. Try to replace them with underscores.

Add Decimal support

8e13586

Add doc, clean up import

4bdb340

Treat bool as Gauge too

e8aa7a7

The Enum type is being dropped in multiprocess mode and Gauge is perfectly enough to represent booleans

Remove unused import

95da931

kdowolski-reef reviewed Jun 11, 2022

View reviewed changes

...utter.django_project_name}}/{{cookiecutter.django_default_app_name}}/constance_prometheus.py Outdated Show resolved Hide resolved

ppolewicz reviewed Jun 11, 2022

View reviewed changes

Fix issues pointed out in code review

a1b416d

Make two functions to store metrics

c02cd1c

As ideated in the CR, there could be two functions: - one which stores a single monitored variable, - another which stores all of them. This commit implements them.

Add type hints

8f48671

mpnowacki-reef reviewed Jun 16, 2022

View reviewed changes

Simplify the str handling code

016937f

msalaban-reef mentioned this pull request Jun 19, 2022

Constance metrics+datetime #78

Open

Remove unused import

740fd69

Manually recycle the metric if old value is used

15cd1fc

THIS IS A HACK: To avoid key clash, try to reuse an existing metric by using undocumented mapping in the registry.

Remove unused imports

38690b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constance metrics #73

Constance metrics #73

msalaban-reef commented Jun 8, 2022

lgtm-com bot commented Jun 8, 2022

lgtm-com bot commented Jun 8, 2022

ppolewicz Jun 11, 2022

mpnowacki-reef Jun 16, 2022 •

edited

Loading

mpnowacki-reef Jun 16, 2022 •

edited

Loading

msalaban-reef Jun 20, 2022

ppolewicz Jun 11, 2022

msalaban-reef Jun 14, 2022

lgtm-com bot commented Jun 14, 2022

lgtm-com bot commented Jun 14, 2022

msalaban-reef commented Jun 14, 2022

lgtm-com bot commented Jun 14, 2022

msalaban-reef commented Jun 15, 2022

ppolewicz commented Jun 15, 2022

mpnowacki-reef Jun 16, 2022

msalaban-reef Jun 19, 2022

msalaban-reef Jun 20, 2022

lgtm-com bot commented Jun 19, 2022

lgtm-com bot commented Jun 19, 2022

lgtm-com bot commented Jun 20, 2022

lgtm-com bot commented Jun 20, 2022

msalaban-reef commented Jul 14, 2022

Constance metrics #73

Are you sure you want to change the base?

Constance metrics #73

Conversation

msalaban-reef commented Jun 8, 2022

lgtm-com bot commented Jun 8, 2022

lgtm-com bot commented Jun 8, 2022

ppolewicz Jun 11, 2022

Choose a reason for hiding this comment

mpnowacki-reef Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

mpnowacki-reef Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

msalaban-reef Jun 20, 2022

Choose a reason for hiding this comment

ppolewicz Jun 11, 2022

Choose a reason for hiding this comment

msalaban-reef Jun 14, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jun 14, 2022

lgtm-com bot commented Jun 14, 2022

msalaban-reef commented Jun 14, 2022

lgtm-com bot commented Jun 14, 2022

msalaban-reef commented Jun 15, 2022

ppolewicz commented Jun 15, 2022

mpnowacki-reef Jun 16, 2022

Choose a reason for hiding this comment

msalaban-reef Jun 19, 2022

Choose a reason for hiding this comment

msalaban-reef Jun 20, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jun 19, 2022

lgtm-com bot commented Jun 19, 2022

lgtm-com bot commented Jun 20, 2022

lgtm-com bot commented Jun 20, 2022

msalaban-reef commented Jul 14, 2022

mpnowacki-reef Jun 16, 2022 •

edited

Loading

mpnowacki-reef Jun 16, 2022 •

edited

Loading