v2: Refactor metrics interceptor and fix tests #413

ash2k · 2021-04-21T01:11:18Z

I decided to have a look at the v2 branch as I'm interested in better Prometheus metrics. I found that stuff was in a very weird state so I tried to fix, simplify and clean it all up. See inline comments for some details. I have also found that tests fail and are not executed on macOS (#411).

Closes #412.

google-cla · 2021-04-21T01:11:24Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

codecov-commenter · 2021-04-21T01:13:29Z

Codecov Report

Merging #413 (3b0fd77) into v2 (a79558a) will decrease coverage by 27.52%.
The diff coverage is 38.77%.

❗ Current head 3b0fd77 differs from pull request most recent head 3bfba60. Consider uploading reports for the commit 3bfba60 to get more accurate results

@@             Coverage Diff             @@
##               v2     #413       +/-   ##
===========================================
- Coverage   83.58%   56.06%   -27.53%     
===========================================
  Files          30       38        +8     
  Lines         932     1336      +404     
===========================================
- Hits          779      749       -30     
- Misses        114      515      +401     
- Partials       39       72       +33

Impacted Files	Coverage Δ
chain.go	`0.00% <ø> (-90.91%)`	⬇️
interceptors/auth/auth.go	`100.00% <ø> (ø)`
interceptors/auth/metadata.go	`100.00% <ø> (ø)`
interceptors/ratelimit/ratelimit.go	`50.00% <0.00%> (-50.00%)`	⬇️
interceptors/tags/fieldextractor.go	`13.79% <0.00%> (-71.51%)`	⬇️
testing/testpb/interceptor_suite.go	`0.00% <0.00%> (ø)`
testing/testpb/test.manual_extractfields.pb.go	`0.00% <0.00%> (ø)`
testing/testpb/test.manual_validator.pb.go	`0.00% <0.00%> (ø)`
testing/testpb/test.pb.go	`36.92% <36.92%> (ø)`
interceptors/logging/payload.go	`67.18% <42.30%> (-15.43%)`	⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0df73fc...3bfba60. Read the comment docs.

ash2k · 2021-04-21T01:13:38Z

@googlebot I signed it!

interceptors/client.go

ash2k · 2021-04-21T01:16:38Z

interceptors/client_test.go

@@ -50,7 +50,7 @@ func (m *mockReportable) Equal(t *testing.T, expected []*mockReport) {
 				require.NoError(t, err)
 				continue
 			}
-			require.Equal(t, expected[i].postCalls[k].Error(), err.Error(), "%v %v", i, k)
+			require.EqualError(t, err, expected[i].postCalls[k].Error(), "%v %v", i, k)


If there is a bug and err is nil, then this line panics. Now it does not and fails the test cleanly.

👍🏽 Thanks a lot

ash2k · 2021-04-21T01:17:19Z

interceptors/client_test.go

@@ -276,7 +276,7 @@ func (s *ClientInterceptorTestSuite) TestListReporting() {
 		typ:             ServerStream,
 		svcName:         testpb.TestServiceFullName,
 		methodName:      "PingList",
-		postCalls:       []error{io.EOF},
+		postCalls:       []error{nil},


Because of the change in interceptors/client.go, it's called with nil now.

ash2k · 2021-04-21T01:17:50Z

interceptors/client_test.go

@@ -329,7 +329,9 @@ func (s *ClientInterceptorTestSuite) TestBiStreamingReporting() {
 			if err == io.EOF {
 				break
 			}
-			require.NoError(s.T(), err, "reading pingStream shouldn't fail")


It is incorrect to use require in a goroutine other than the main test goroutine.

ash2k · 2021-04-21T01:18:12Z

interceptors/client_test.go

@@ -340,12 +342,12 @@ func (s *ClientInterceptorTestSuite) TestBiStreamingReporting() {
 	require.NoError(s.T(), ss.CloseSend())
 	wg.Wait()

-	require.EqualValues(s.T(), count, 100, "Number of received msg on the wire must match")
+	require.EqualValues(s.T(), 100, count, "Number of received msg on the wire must match")


Put actual and expected values where they should be.

Wow, thanks for those detailed comments (: Next time, no need for this it's very clear! 💪🏽

ash2k · 2021-04-21T01:27:40Z

providers/openmetrics/go.mod

@@ -11,3 +11,5 @@ require (
 	google.golang.org/grpc v1.35.0
 	google.golang.org/protobuf v1.25.0
 )
+
+replace github.com/grpc-ecosystem/go-grpc-middleware/v2 => ../..


I'm not sure how to test it otherwise if changes are made to the outer module and to providers.

I think it'll have to be merged in two steps.

If this PR is approved I can split it into two - one updating the base and another one updating the provider. I also have a few more things I'd like to cleanup in follow ups.

Sounds good to me, will give @bwplotka a few days to have a look but I'm otherwise happy to merge this on our own.

Yup, this is the problem with multi-modules.

One thing we could add is to add some tooling that e.g makes commit with this PR on your fork, then creates commit on top of it, so it's at least not broken on master, if you just pull openmetrics module. Local path approach can break users on main. It's fine for now (dev part of v2) but we might want to improve this process. LGTM for now

ash2k · 2021-04-21T01:29:38Z

providers/openmetrics/reporter.go

 }

 func (rep *reportable) ServerReporter(ctx context.Context, _ interface{}, typ interceptors.GRPCType, service string, method string) (interceptors.Reporter, context.Context) {
-	m := NewServerMetrics(rep.registry)


This is super weird. So we're creating a new metrics holder here, we do not register it anywhere and just use it as is. This is just broken - there is no way these metrics are going to be scraped.

ash2k · 2021-04-21T01:30:54Z

providers/openmetrics/server_interceptor.go

@@ -1,35 +1,21 @@
 package metrics


Server metrics and interceptor are refactored in the same way as client metrics and interceptor. All comments apply, not repeating them.

ash2k · 2021-04-21T01:32:15Z

providers/openmetrics/server_test.go

 	resp := httptest.NewRecorder()
 	req, err := http.NewRequest("GET", "/", nil)
 	require.NoError(t, err, "failed creating request for Prometheus handler")

-	promhttp.Handler().ServeHTTP(resp, req)
+	promhttp.HandlerFor(reg, promhttp.HandlerOpts{}).ServeHTTP(resp, req)


Use the explicitly passed registry here rather than a global one. Test should be self-contained and hermetic, it shouldn't depend on globals.

I agree, thanks for this

ash2k · 2021-04-21T01:33:19Z

providers/openmetrics/server_test.go

@@ -292,6 +293,7 @@ func toFloat64HistCount(h prometheus.Observer) uint64 {
 }

 func requireValue(t *testing.T, expect int, c prometheus.Collector) {
+	t.Helper()


That way error traces when a test fails lead to the line where this function was invoked, not into this function. Convenience thing, helped me to navigate failing tests.

johanbrandhorst

This looks generally good to me, thanks for taking the time. I'll hold off on approving as this is @bwplotka's area of expertise.

interceptors/client.go

johanbrandhorst · 2021-04-21T12:30:44Z

providers/openmetrics/client_test.go

-	requireValueHistCount(s.T(), 1, DefaultClientMetrics.clientHandledHistogram.WithLabelValues("unary", "mwitkow.testproto.TestService", "PingEmpty"))
+	requireValue(s.T(), 1, s.clientMetrics.clientStartedCounter.WithLabelValues("unary", "providers.openmetrics.testproto.v1.TestService", "PingEmpty"))
+	requireValue(s.T(), 1, s.clientMetrics.clientHandledCounter.WithLabelValues("unary", "providers.openmetrics.testproto.v1.TestService", "PingEmpty", "OK"))
+	requireValueHistCount(s.T(), 1, s.clientMetrics.clientHandledHistogram.WithLabelValues("unary", "providers.openmetrics.testproto.v1.TestService", "PingEmpty"))


johanbrandhorst · 2021-04-21T12:31:42Z

providers/openmetrics/go.mod

@@ -11,3 +11,5 @@ require (
 	google.golang.org/grpc v1.35.0
 	google.golang.org/protobuf v1.25.0
 )
+
+replace github.com/grpc-ecosystem/go-grpc-middleware/v2 => ../..


I think it'll have to be merged in two steps.

bwplotka

Thanks for this, LGTM

I agree that the openmetrics PR might have been prematurely merged.. Yet it was kind of stated to merge a bigger chunk of work for later to iterate... yet iteration never happened and the code on main was unusable. It's totally on me, sorry for not paying close attention during the review process. Cc @yashrsharma44 for learning too on this (:

Let's make sure to keep the quality bar much higher next time.

bwplotka · 2021-04-23T18:28:04Z

interceptors/client_test.go

@@ -50,7 +50,7 @@ func (m *mockReportable) Equal(t *testing.T, expected []*mockReport) {
 				require.NoError(t, err)
 				continue
 			}
-			require.Equal(t, expected[i].postCalls[k].Error(), err.Error(), "%v %v", i, k)
+			require.EqualError(t, err, expected[i].postCalls[k].Error(), "%v %v", i, k)


👍🏽 Thanks a lot

bwplotka · 2021-04-23T18:28:47Z

providers/openmetrics/client_interceptor.go

 // UnaryClientInterceptor is a gRPC client-side interceptor that provides Prometheus monitoring for Unary RPCs.
-func UnaryClientInterceptor(clientRegister openmetrics.Registerer) grpc.UnaryClientInterceptor {
+func UnaryClientInterceptor(clientMetrics *ClientMetrics) grpc.UnaryClientInterceptor {


Agree, cc @yashrsharma44 for learning experience (:

bwplotka · 2021-04-23T18:32:37Z

interceptors/client_test.go

@@ -340,12 +342,12 @@ func (s *ClientInterceptorTestSuite) TestBiStreamingReporting() {
 	require.NoError(s.T(), ss.CloseSend())
 	wg.Wait()

-	require.EqualValues(s.T(), count, 100, "Number of received msg on the wire must match")
+	require.EqualValues(s.T(), 100, count, "Number of received msg on the wire must match")


Wow, thanks for those detailed comments (: Next time, no need for this it's very clear! 💪🏽

bwplotka · 2021-04-23T18:44:57Z

providers/openmetrics/go.mod

@@ -11,3 +11,5 @@ require (
 	google.golang.org/grpc v1.35.0
 	google.golang.org/protobuf v1.25.0
 )
+
+replace github.com/grpc-ecosystem/go-grpc-middleware/v2 => ../..


Yup, this is the problem with multi-modules.

One thing we could add is to add some tooling that e.g makes commit with this PR on your fork, then creates commit on top of it, so it's at least not broken on master, if you just pull openmetrics module. Local path approach can break users on main. It's fine for now (dev part of v2) but we might want to improve this process. LGTM for now

bwplotka · 2021-04-23T18:46:23Z

providers/openmetrics/server_test.go

 	resp := httptest.NewRecorder()
 	req, err := http.NewRequest("GET", "/", nil)
 	require.NoError(t, err, "failed creating request for Prometheus handler")

-	promhttp.Handler().ServeHTTP(resp, req)
+	promhttp.HandlerFor(reg, promhttp.HandlerOpts{}).ServeHTTP(resp, req)


I agree, thanks for this

yashrsharma44 · 2021-04-26T08:03:37Z

Thanks, @ash2k for such great work!

I was the original author for introducing the changes for openmetrics and I agree that the changes were not suitable to be merged. Thanks for such a comprehensive review and this PR might be a good opportunity for me to write good Go code 😄

Refactor metrics interceptor and fix tests

3bfba60

ash2k commented Apr 21, 2021

View reviewed changes

johanbrandhorst reviewed Apr 21, 2021

View reviewed changes

bwplotka approved these changes Apr 23, 2021

View reviewed changes

bwplotka merged commit f60016a into grpc-ecosystem:v2 Apr 23, 2021

ash2k deleted the refactor-metrics branch April 23, 2021 22:59

yashrsharma44 mentioned this pull request Apr 26, 2021

v2: Not all tests are executed on macOS #411

Closed

aimuz mentioned this pull request Mar 8, 2023

docs: add logging.InjectFields usage description #540

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2: Refactor metrics interceptor and fix tests #413

v2: Refactor metrics interceptor and fix tests #413

ash2k commented Apr 21, 2021

google-cla bot commented Apr 21, 2021

codecov-commenter commented Apr 21, 2021 •

edited

ash2k commented Apr 21, 2021

ash2k Apr 21, 2021

bwplotka Apr 23, 2021

ash2k Apr 21, 2021

ash2k Apr 21, 2021

ash2k Apr 21, 2021

bwplotka Apr 23, 2021

ash2k Apr 21, 2021

johanbrandhorst Apr 21, 2021

ash2k Apr 22, 2021

johanbrandhorst Apr 22, 2021

bwplotka Apr 23, 2021

ash2k Apr 21, 2021

ash2k Apr 21, 2021

ash2k Apr 21, 2021

bwplotka Apr 23, 2021

ash2k Apr 21, 2021

johanbrandhorst left a comment

johanbrandhorst Apr 21, 2021

johanbrandhorst Apr 21, 2021

bwplotka left a comment

bwplotka Apr 23, 2021

bwplotka Apr 23, 2021

bwplotka Apr 23, 2021

bwplotka Apr 23, 2021

bwplotka Apr 23, 2021

yashrsharma44 commented Apr 26, 2021

v2: Refactor metrics interceptor and fix tests #413

v2: Refactor metrics interceptor and fix tests #413

Conversation

ash2k commented Apr 21, 2021

google-cla bot commented Apr 21, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

codecov-commenter commented Apr 21, 2021 • edited

Codecov Report

ash2k commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashrsharma44 commented Apr 26, 2021

codecov-commenter commented Apr 21, 2021 •

edited