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

upgrade dependency on prometheus-client #1

Open
maszczyn opened this issue Jan 21, 2023 · 3 comments
Open

upgrade dependency on prometheus-client #1

maszczyn opened this issue Jan 21, 2023 · 3 comments

Comments

@maszczyn
Copy link

Hi there :)

I've noticed a recent commit, in which you introduced a restriction on prometheus-client dependency - now it's gotta be at most version 0.12. Looking at the contents of 0.13 release of prometheus-client I see no breaking changes or anything else that would justify this restriction in the package you maintain. Where does this restriction come from? Is there anything that you know breaks when version 0.13 or more recent is used?

I am using previous release of prometheus-distributed-client (1.2.1) with most recent prometheus-client (0.15 at the moment) and it seems to work just fine, although I am likely not using all features.

@jaesivsm
Copy link
Member

Hello !

About a year ago we noticed a change in the prototype of CollectorRegistry.collect and calling on registry.collect() would raise something like :

File "/usr/lib/python3.7/site-packages/prometheus_client/metrics.py", line 101, in collect
    for suffix, labels, value, timestamp, exemplar in self._samples():
ValueError: not enough values to unpack (expected 5, got 3)

We proceeded with a dirty fix, which was to fix the prometheus-client dependency to anything below 0.12. Recently I took upon me to fix the dependency more explicitly.

Upon researching the issue we noticed that other prototypes (Histogram.observe for example) got changed so the work need to ensure full compatibility isn't negligible. But it's something I'd like to get onto in the near future.

If you didn't experience any trouble at all, I think your best option in the mean time is to stick with 1.2.1 :)

@maszczyn
Copy link
Author

Now it's all clear - thank you for the explanation :)

I wasn't aware of the implicit dependency, and I did bump onto the error you mentioned, but instead of lowering the prometheus-client version, I did a workaround of my own. I did it quite long ago and forgot about it, but the error you quoted made me recall it's there.

The "works for me" workaround code for Counter is:

import prometheus_distributed_client


class Counter(prometheus_distributed_client.Counter):
    def _multi_samples(self):
        for sample in super()._multi_samples():
            yield sample + (None, None)

    _child_samples = _multi_samples

and it's very similar for other metric types. This is definitely not a proper solution, but it was good enough in my case.

I propose to leave this issue open until the dependency can be safely upgraded.

@jaesivsm
Copy link
Member

Thanks for the input ! I haven't had time to work on a fix yet but that patch is definitely a step forward in that direction.
As the upstream lib recently published a new version, I'll try to work on a fix based on the 0.0.16

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

No branches or pull requests

2 participants