search

lchenn / py-grpc-prometheus

Python gRPC Prometheus
Apache License 2.0
48 stars 25 forks source link

Update metrics definition and implementation to be in line with go-grpc-prometheus #11

Closed Tiernan-Stapleton closed 3 years ago

Tiernan-Stapleton commented 3 years ago

Update the metrics found in client_metrics.py and server_metrics.py to match the metric definitions found in https://github.com/grpc-ecosystem/go-grpc-prometheus/blob/master/client_metrics.go, and https://github.com/grpc-ecosystem/go-grpc-prometheus/blob/master/server_metrics.go.

Implement the optional turning on of histogram metrics on gRPC calls, similar to https://github.com/grpc-ecosystem/go-grpc-prometheus#histograms.

Update the README to reflect these changes.

lchenn commented 3 years ago

Thanks for the pull request. Overall, it makes sense. Apparently the python/go/java are not consistent. This will bring things to the next step. My only concern is the backward compatibility. If someone depends on a metric which is renamed now, then they are broken.

Tiernan-Stapleton commented 3 years ago

Hmm, hadn't considered that. One solution one be I could add a legacy value to the interceptor initialisers, similar to how I have it with the histogram enabling. Have the legacy default to false, but if its true the old metrics would be used.

As long as this change is made very prominent in the release notes I don't think backwards compatibility will be too much of an issue.

Tiernan-Stapleton commented 3 years ago

@lchenn do these changes work for you?

lchenn commented 3 years ago

The general approach looks good to me, I will take a closer look near the weekend. Thank you for checking!