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

Django-prometheus breaks due to prometheus-client python package release 0.4.0 on Oct 04 2018 #83

Closed
billaram opened this issue Oct 5, 2018 · 10 comments · Fixed by #100

Comments

@billaram
Copy link

billaram commented Oct 5, 2018

Issue

I m using uWSGI and django 1.11.14 and django_proometheus in multi-process mode

uwsgi.ini

env = prometheus_multiproc_dir=/path/to/django_metrics

wsgi.py

from prometheus_client import core
import uwsgi
# Use uwsgi's worker_id rather than system pids
core._ValueClass = core._MultiProcessValue(_pidFunc=uwsgi.worker_id)

Note: reverting to 0.3.1 works fine

@crccheck
Copy link

crccheck commented Oct 9, 2018

I'm using Waitress and prometheus-client 0.4.1 is still working for me

@korfuri
Copy link
Owner

korfuri commented Oct 10, 2018 via email

@manics
Copy link

manics commented Oct 17, 2018

Looks like the unit tests are failing: #87

  File "/home/travis/build/korfuri/django-prometheus/django_prometheus/testutils.py", line 48, in getMetricFromFrozenRegistry
    for n, l, value in metric.samples:
ValueError: too many values to unpack (expected 3)

I think this is the relevant PR: prometheus/client_python#300
I haven't found the actual change, but metric.samples is being used differently e.g. https://github.com/prometheus/client_python/pull/300/files#diff-52e98fdb8931e88f7d37feb48ba6ded2L51

@NightTsarina
Copy link

Hi!

Any news on this?

I am one of the Debian maintainers for this package and it currently risks being excluded from the next release because currently it fails to build. I am tempted to patch the package as per #88, but that is lacking the changes requested in #84. I could implement those, but I would not like to end up with different metrics names than the ones you end up with! (For example, following that change, I'd also remove _total_ from other metric names...

@bz2
Copy link
Contributor

bz2 commented Jan 21, 2019

I suggest this should be fixed by pinning the prometheus_client version to <0.4.0 now, then releasing a new large version bump with updated metric names and so on matching the new scheme that is >=0.4.0

bz2 added a commit to bz2/django-prometheus that referenced this issue Jan 21, 2019
Fixes korfuri#83

Simple dependency change that will fix tests and can be released as
a working package. Follow up can adapt to the interface and metric
name changes in 0.4.0 and depend on that version.
@NightTsarina
Copy link

@bz2 Any ETA on that new release? Right now the situation (in Debian) is that prometheus_client is at 0.5.0, and that this package fails to build (normally, we don't keep old libraries around). I would like to patch it so it can be included in the next stable release, but the deadline for that is in a couple of weeks.

@bz2
Copy link
Contributor

bz2 commented Jan 28, 2019

@TheTincho I'd happy do the work in time for buster freeze, but don't have the ability to make a release. I have poked @korfuri by email to see if he wants help with maintaining this package.

Also checked the mysqlclient-python that needed fixing as well, but isn't a dep in the Debian packaging, and doesn't have that updated fork packaged anyway yet?

@NightTsarina
Copy link

@bz2 I would be happy to work with an unreleased patch. Ideally, the metric names would stay the same in the release later, so users don't get their timeseries screwed up in the next update, but even if we don't get that it would be better than no package.

I don't know what's the story with the mysqlclient library.. It indeed is not a dependency now, would it be in the next release?

@manics
Copy link

manics commented Jan 28, 2019

I think MySQL is an optional dependency: https://github.com/korfuri/django-prometheus#monitoring-your-databases

@bz2
Copy link
Contributor

bz2 commented Jan 28, 2019

@TheTincho Doesn't need to be a debian binary dependency - is a driver that provides for django.db.backends.mysql, and is needed for running the end to end tests (which I guess are not plugged in to dep8).

korfuri pushed a commit that referenced this issue Jun 13, 2019
* Pin prometheus-client to <0.4.0

Fixes #83

Simple dependency change that will fix tests and can be released as
a working package. Follow up can adapt to the interface and metric
name changes in 0.4.0 and depend on that version.

* Pin mysqlclient to <1.4

Upstream changes are incompatible with older Django versions, see:

    PyMySQL/mysqlclient#306
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.

6 participants