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

Support Various Prometheus Client Versions #129

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Support Various Prometheus Client Versions #129

merged 1 commit into from
Apr 7, 2022

Conversation

kojiromike
Copy link
Contributor

@kojiromike kojiromike commented Apr 7, 2022

closes #128
closes #127

also see #125
also see #126

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 84.718% when pulling 943731c on kojiromike:choose-your-own-adventure into 02b2ea3 on rycus86:master.

@rycus86
Copy link
Owner

rycus86 commented Apr 7, 2022

Legend, thanks a lot! I'll bump the version (and restore older Python version support perhaps) and tag it for release.

@kojiromike
Copy link
Contributor Author

@rycus86 do you have a way I can add both versions of the client to the test matrix?

@rycus86
Copy link
Owner

rycus86 commented Apr 7, 2022

Do you mean test with both versions of prometheus_client? I could imagine adding some pinning to the requirements.txt file before starting unit/integration tests, but I don't think it's necessary to be honest.

@rycus86 rycus86 merged commit d423412 into rycus86:master Apr 7, 2022
@kojiromike
Copy link
Contributor Author

Do you mean test with both versions of prometheus_client?

Right, it looks like you have a build matrix in Actions for testing different Python versions. I was just suggesting we could add a matrix for testing multiple versions of other dependencies, if you want.

@rycus86
Copy link
Owner

rycus86 commented Apr 7, 2022

we could add a matrix for testing multiple versions of other dependencies

Yes, we could but I don't think it's necessary (do you know other packages that do similar things?), but can reconsider later if this becomes a regular problem.

PS: love the branch name 👍

@csmarchbanks
Copy link

I just wanted to chime in here, sorry about the hassle! The rename was not meant as a breaking change, and I plan to revert back to the old name in prometheus/client_python#796. It looks like with your fix will be fine. If you see breaking changes in prometheus_client in the future feel free to open an issue and I will try to resolve it as quickly as I can!

@Starkteetje
Copy link

Starkteetje commented Apr 8, 2022

Note that reverting the name change is a breaking change again for everyone pinning prometheus-flask-exporter 0.20 and not pinning prometheus-client at the same time.

@rycus86
Copy link
Owner

rycus86 commented Apr 8, 2022

Which is resolved in 0.20.1 :/

@Starkteetje
Copy link

Maybe I'm misunderstanding you, but 0.20.1 still doesn't seem to pin the prometheus-client version either in https://github.com/rycus86/prometheus_flask_exporter/blob/0.20.1/requirements.txt or https://github.com/rycus86/prometheus_flask_exporter/blob/0.20.1/setup.py

Compare i.e. with https://github.com/pallets/flask/blob/main/setup.py where the accepted versions are declared

Running pip install prometheus-flask-exporter this leads to Requirement already satisfied: prometheus-client in ./lib/python3.8/site-packages (from prometheus-flask-exporter) (0.14.0) but Requirement already satisfied: Jinja2>=3.0 in ./lib/python3.8/site-packages (from flask->prometheus-flask-exporter) (3.1.1). So for prometheus-client any version is pulled, but for Jinja2 it needs to be >=3 due to the requirement in Flask's setup.py

Though to be fair >= wouldn't have prevented the incompatibility in this case as the breaking change was introduced without increasing the major version

@kojiromike
Copy link
Contributor Author

@Starkteetje the fix in 0.20.1 does not require pinning. It is designed to work with either version-range of prometheus-client. If prometheus-client reverts the incompatibility, then 0.20.1 should still work.

@Starkteetje
Copy link

I see. That makes sense, especially in light of the change maybe being reverted.
However, the root cause is not tackled by this. A breaking change in Flask (less of an issue as its probably a direct dependency of whoever is using prometheus_flask_exporter) or a breaking change in prometheus-client are both likely to break existing setups. Pinning the version with ~= would mitigate this if both dependencies adhere to semver (and prometheus-client had major version 1), so I'd still see value in that.
Pinning with == would resolve the issue completely, but that might be too strong of a dependency 🤷‍♂️

@rycus86
Copy link
Owner

rycus86 commented Apr 9, 2022

I don't think I want to start adding requirements on versions of dependencies here, I don't want this library to prescribe what versions it can work with. As long as we can reasonably make it work with different/multiple versions, we should be OK?

@rycus86
Copy link
Owner

rycus86 commented Apr 9, 2022

I just wanted to chime in here, sorry about the hassle! The rename was not meant as a breaking change, and I plan to revert back to the old name in prometheus/client_python#796. It looks like with your fix will be fine. If you see breaking changes in prometheus_client in the future feel free to open an issue and I will try to resolve it as quickly as I can!

No worries, and thanks for looking into it! ❤️ I didn't even realize this library is on your radar. 😄

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 this pull request may close these issues.

ImportError: cannot import name 'choose_encoder' from 'prometheus_client.exposition'
5 participants