search

claws / aioprometheus

A Prometheus Python client library for asyncio-based applications
175 stars 21 forks source link

Optional protoc #42

Closed hellysmile closed 4 years ago

hellysmile commented 4 years ago

As discussed https://github.com/claws/aioprometheus/issues/41

hellysmile commented 4 years ago

@claws ready for review

claws commented 4 years ago

This looks great. I didn't quite realise how many files it would impact. There are some areas in the Sphinx docs that could be updated to reflect this change (for example the curl example in index.rst that fetches a binary response should indicate that it needs the optional extra installed to work) - but I'm happy to do these changes if you want.

I have an aversion to the optional label 'protoc' that you chose. It makes me think it will install the protoc compiler - possibly just because I am familiar such a tool exists. I'd like the optional extra label to more accurately convey that it is installing the prometheus-metrics-proto package which provides binary response support. I'd suggest something like protobuf or just binary. I think protobuf suffers from the same problem in that it is an existing Python package. So perhaps use binary as the extras label that will include support for binary format responses, unless you have an alternative suggestion?

hellysmile commented 4 years ago

@claws updated according to your comments