incrementing grpc_server_msg_received_total and grpc_server_msg_sent_total in unary methods

[metasyn]

👋 hello @lchenn

firstly thank you for writing this library 🙇

when comparing the py-grpc-prometheus library to the go-grpc-prometheus library, you can see that the go version increments the grpc_server_msg_received_total and grpc_server_msg_sent_total metrics for unary and streaming methods. in the py-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 the grpc_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

[lchenn]

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.

[metasyn]

Great! We will fashion a PR the changes with some references to why we think this is correct. Thanks for being receptive.

[metasyn]

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:

• why UnaryClientInterceptor use SentMessage() and ReceivedMessage() grpc-ecosystem/go-grpc-prometheus#81

Anyhow, I will close this issue as I think I didn't fully understand. Thanks!

[lchenn]

Thanks @metasyn for the research and drawing the conclusion!