Closed metasyn closed 2 years ago
Hey @metasyn , thanks for your feedback. I created this library to mimic the behavior with the java version. Haven't gotten a chance to compare the difference yet. If you think it's the right behavior, feel encourage to put a pull request.
Great! We will fashion a PR the changes with some references to why we think this is correct. Thanks for being receptive.
Upon looking further, I think that this may actually be a bug with the go-grpc-prometheus package.
In the java version of the package - there are some assertions that unary methods do not increment the grpc_server_msg_received_total
and grpc_server_msg_sent_total
metrics - see here.
However in the golang version, those assertions don't exist - see here. By applying the following patch and running the go tests:
diff --git a/server_test.go b/server_test.go
index 5ee7456..9ee563c 100644
--- a/server_test.go
+++ b/server_test.go
@@ -133,12 +133,20 @@ func (s *ServerInterceptorTestSuite) TestRegisterPresetsStuff() {
func (s *ServerInterceptorTestSuite) TestUnaryIncrementsMetrics() {
_, err := s.testClient.PingEmpty(s.ctx, &pb_testproto.Empty{}) // should return with code=OK
require.NoError(s.T(), err)
+
+ requireValue(s.T(), 1, DefaultServerMetrics.serverStreamMsgReceived.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty"))
+ requireValue(s.T(), 1, DefaultServerMetrics.serverStreamMsgSent.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty"))
+
requireValue(s.T(), 1, DefaultServerMetrics.serverStartedCounter.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty"))
requireValue(s.T(), 1, DefaultServerMetrics.serverHandledCounter.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty", "OK"))
requireValueHistCount(s.T(), 1, DefaultServerMetrics.serverHandledHistogram.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty"))
_, err = s.testClient.PingError(s.ctx, &pb_testproto.PingRequest{ErrorCodeReturned: uint32(codes.FailedPrecondition)}) // should return with code=FailedPrecondition
require.Error(s.T(), err)
+
+ requireValue(s.T(), 1, DefaultServerMetrics.serverStreamMsgReceived.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingError"))
+ requireValue(s.T(), 0, DefaultServerMetrics.serverStreamMsgSent.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingError"))
+
requireValue(s.T(), 1, DefaultServerMetrics.serverStartedCounter.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingError"))
requireValue(s.T(), 1, DefaultServerMetrics.serverHandledCounter.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingError", "FailedPrecondition"))
requireValueHistCount(s.T(), 1, DefaultServerMetrics.serverHandledHistogram.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingError"))
But closer reading of the code makes me think this is unintentional. I think this leads me to believe that the issue is actually in the go package, and that both the java and your version are correct.
I see some other people were also confused by this difference:
Anyhow, I will close this issue as I think I didn't fully understand. Thanks!
Thanks @metasyn for the research and drawing the conclusion!
👋 hello @lchenn
firstly thank you for writing this library 🙇
when comparing the
py-grpc-prometheus
library to thego-grpc-prometheus
library, you can see that the go version increments thegrpc_server_msg_received_total
andgrpc_server_msg_sent_total
metrics for unary and streaming methods. in thepy-grc-prometheus
library, it seems that we only increment these metrics if the method itself is a streaming type of method.Compare the go version: https://github.com/grpc-ecosystem/go-grpc-prometheus/blob/82c243799c991a7d5859215fba44a81834a52a71/server_metrics.go#L103-L116
To the python version (received): https://github.com/lchenn/py-grpc-prometheus/blob/master/py_grpc_prometheus/prometheus_server_interceptor.py#L53-L59
And here (sent): https://github.com/lchenn/py-grpc-prometheus/blob/master/py_grpc_prometheus/prometheus_server_interceptor.py#L69-L77
could we update this so that the msg received/sent metrics are also updated for unary methods? my understanding is that the
grpc_server_msg_received_total
should always be incremented, before going into the handler. if the handler function finishes without error, we increment the handler metric. then, finally, just before our "final" return, we increment thegrpc_server_msg_received_total
. this understanding might be incorrect, but that is what i thought from reading the go version of the prometheus interceptor.best, xander