balancer: remove grpc.lb.pick_first.* metrics#9215
Conversation
…ubchannel metrics
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9215 +/- ##
==========================================
+ Coverage 83.26% 83.31% +0.04%
==========================================
Files 420 420
Lines 34028 34021 -7
==========================================
+ Hits 28333 28343 +10
+ Misses 4267 4257 -10
+ Partials 1428 1421 -7
🚀 New features to boost your workflow:
|
|
@mbissa @arjan-bal @Pranjali-2501 @easwars Sorry for pinging everyone like this, but I'd really appreciate it if someone could take a quick look and review this when you have a chance. Thanks! |
| // These are example experimental gRPC metrics, which are disabled | ||
| // by default and must be explicitly enabled. For the full, | ||
| // up-to-date list of metrics, see: | ||
| // https://grpc.io/docs/guides/opentelemetry-metrics/#instruments | ||
| Metrics: opentelemetry.DefaultMetrics().Add( | ||
| "grpc.lb.pick_first.connection_attempts_succeeded", | ||
| "grpc.lb.pick_first.connection_attempts_failed", | ||
| ), |
There was a problem hiding this comment.
Why is this different to the change in examples/features/opentelemetry/client/main.go?
There was a problem hiding this comment.
The grpc.subchannel.* metrics are client-side only. The client example still needs to opt into the renamed metrics, but the server does not create subchannels or outbound connection attempts, so the old block there was just a no-op. That’s why I renamed it on the client and removed it on the server.
| if got, _ := tmr.Metric("grpc.lb.subchannel.connection_attempts_succeeded"); got != 0 { | ||
| t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.subchannel.connection_attempts_succeeded", got, 0) |
There was a problem hiding this comment.
Interesting. Was this broken all this while then, @mbissa ?
There was a problem hiding this comment.
Yes. looks like it. I think when tmr.Metric is queried with the typo'd name, it does not find any recorded values, returning (0, false). Since the test code only checked got (which defaults to 0 when not found) and asserted got != 0, the condition 0 != 0 evaluated to false, and the test incorrectly passed instead of failing.
| if holds[1].Wait(ctx) != true { | ||
| t.Fatalf("Timeout waiting for server %d with address %q to be contacted", 1, addrs[1]) | ||
| } | ||
| if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 1 { |
There was a problem hiding this comment.
@mbissa : For all of these removed lines, what was the original plan? Would the pick_first tests continue to track the subchannel metrics or did we plan to have other tests that would track with these metrics?
There was a problem hiding this comment.
The plan was to move the tests to different place which is more apt for subchannel and not keep them in pickfirst.
| // TestPickFirstMetrics verifies that a pick_first channel emits the correct | ||
| // subchannel metrics on connect and disconnect. |
There was a problem hiding this comment.
Nit: "a pick_first channel" is very confusing. What we probably want is "a gRPC channel configured with the pick_first LB policy".
There was a problem hiding this comment.
This is no longer applicable after the test move. TestPickFirstMetrics was removed, and the OTel coverage now lives in stats/opentelemetry/e2e_test.go.
| // metrics to emit, and makes sure the correct OpenTelemetry metrics atoms emit. | ||
| // TestPickFirstMetricsE2E tests subchannel metrics emitted during a pickfirst | ||
| // connection lifecycle: failure, success, and disconnection. | ||
| func (s) TestPickFirstMetricsE2E(t *testing.T) { |
There was a problem hiding this comment.
@mbissa : I don't quite understand why this test uses the OTel stats handler while the other tests in this file seem to use a test stats handler? Should we consider unifying these tests to use a consistent approach? Or is there value in doing what is being currently done?
There was a problem hiding this comment.
This is the only test that runs E2E and hence using actual Otel plugin. The test stats handler ones are more geared towards checking the label value plumbing rather than the actual metric value. We could in new tests have unified tests - certainly clean it up.
easwars
added
Area: Observability
|
@deepgolani4 can you please move the tests to a separate place in the otel package where we run e2e tests instead of changing just the existing tests. |
@mbissa Moved the OTel e2e metric coverage into stats/opentelemetry/e2e_test.go, and removed the duplicate pickfirst metric tests from balancer/pickfirst. |
3 participants
Fixes #8752
The changes remove the three grpc.lb.pick_first.* metric registrations and their recording calls from pickfirst.go (along with the now-unneeded metricsRecorder/target fields), then update all tests and examples to assert on the equivalent grpc.subchannel.* metrics instead.
RELEASE NOTES:
grpc.lb.pick_first.*metrics.