Skip to content

balancer: remove grpc.lb.pick_first.* metrics #9215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
deepgolani4 wants to merge 6 commits into
base: master
Choose a base branch
from
Open

balancer: remove grpc.lb.pick_first.* metrics #9215

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
643ead2
balancer/pickfirst: remove grpc.lb.pick_first.* metrics in favor of s…
deepgolani4 Jun 30, 2026
cbb45a2
fixed static checks
deepgolani4 Jun 30, 2026
ebca066
empty commits to trigger workflows
deepgolani4 Jul 2, 2026
5fb5a92
move subchannel metric e2e test
deepgolani4 Jul 8, 2026
45070d4
empty commits to trigger workflows
deepgolani4 Jul 8, 2026
b5627a3
Merge branch 'grpc:master' into master
deepgolani4 Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 14 additions & 30 deletions balancer/pickfirst/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ func init() {
}`, pickfirst.Name)
}

// TestPickFirstMetrics tests pick first metrics. It configures a pick first
// balancer, causes it to connect and then disconnect, and expects the
// subsequent metrics to emit from that.
// TestPickFirstMetrics verifies that a pick_first channel emits the correct
// subchannel metrics on connect and disconnect.

@easwars easwars Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "a pick_first channel" is very confusing. What we probably want is "a gRPC channel configured with the pick_first LB policy".

@deepgolani4 deepgolani4 Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer applicable after the test move. TestPickFirstMetrics was removed, and the OTel coverage now lives in stats/opentelemetry/e2e_test.go.

func (s) TestPickFirstMetrics(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
Expand Down Expand Up @@ -97,17 +96,6 @@ func (s) TestPickFirstMetrics(t *testing.T) {
t.Fatalf("EmptyCall() failed: %v", err)
}

if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_succeeded", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_failed", got, 0)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0)
}

// Checking for subchannel metrics as well
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_succeeded", got, 1)
}
Expand All @@ -123,9 +111,6 @@ func (s) TestPickFirstMetrics(t *testing.T) {

ss.Stop()
testutils.AwaitState(ctx, t, cc, connectivity.Idle)
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 1)
}
if got, _ := tmr.Metric("grpc.subchannel.disconnections"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.disconnections", got, 1)
}
Expand Down Expand Up @@ -161,20 +146,19 @@ func (s) TestPickFirstMetricsFailure(t *testing.T) {
t.Fatalf("EmptyCall() passed when expected to fail")
}

if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_succeeded", got, 0)
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_succeeded", got, 0)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_failed", got, 1)
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_failed"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_failed", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0)
if got, _ := tmr.Metric("grpc.subchannel.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.disconnections", got, 0)
}
}

// TestPickFirstMetricsE2E tests the pick first metrics end to end. It
// configures a channel with an OpenTelemetry plugin, induces all 3 pick first
// 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) {

@easwars easwars Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@mbissa mbissa Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
Expand All @@ -199,7 +183,7 @@ func (s) TestPickFirstMetricsE2E(t *testing.T) {
provider := metric.NewMeterProvider(metric.WithReader(reader))
mo := opentelemetry.MetricsOptions{
MeterProvider: provider,
Metrics: opentelemetry.DefaultMetrics().Add("grpc.lb.pick_first.disconnections", "grpc.lb.pick_first.connection_attempts_succeeded", "grpc.lb.pick_first.connection_attempts_failed"),
Metrics: opentelemetry.DefaultMetrics().Add("grpc.subchannel.disconnections", "grpc.subchannel.connection_attempts_succeeded", "grpc.subchannel.connection_attempts_failed"),
}

cc, err := grpc.NewClient(grpcTarget, opentelemetry.DialOption(opentelemetry.Options{MetricsOptions: mo}), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(r))
Expand Down Expand Up @@ -227,7 +211,7 @@ func (s) TestPickFirstMetricsE2E(t *testing.T) {
testutils.AwaitState(ctx, t, cc, connectivity.Idle)
wantMetrics := []metricdata.Metrics{
{
Name: "grpc.lb.pick_first.connection_attempts_succeeded",
Name: "grpc.subchannel.connection_attempts_succeeded",
Description: "EXPERIMENTAL. Number of successful connection attempts.",
Unit: "{attempt}",
Data: metricdata.Sum[int64]{
Expand All @@ -242,7 +226,7 @@ func (s) TestPickFirstMetricsE2E(t *testing.T) {
},
},
{
Name: "grpc.lb.pick_first.connection_attempts_failed",
Name: "grpc.subchannel.connection_attempts_failed",
Description: "EXPERIMENTAL. Number of failed connection attempts.",
Unit: "{attempt}",
Data: metricdata.Sum[int64]{
Expand All @@ -257,7 +241,7 @@ func (s) TestPickFirstMetricsE2E(t *testing.T) {
},
},
{
Name: "grpc.lb.pick_first.disconnections",
Name: "grpc.subchannel.disconnections",
Description: "EXPERIMENTAL. Number of times the selected subchannel becomes disconnected.",
Unit: "{disconnection}",
Data: metricdata.Sum[int64]{
Expand Down
50 changes: 5 additions & 45 deletions balancer/pickfirst/pickfirst.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"google.golang.org/grpc/balancer/pickfirst/internal"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/experimental/balancer/weight"
expstats "google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal/envconfig"
internalgrpclog "google.golang.org/grpc/internal/grpclog"
Expand All @@ -56,30 +55,7 @@ const Name = "pick_first"
// attributes to indicate whether the health listener usage is enabled.
type enableHealthListenerKeyType struct{}

var (
logger = grpclog.Component("pick-first-leaf-lb")
disconnectionsMetric = expstats.RegisterInt64Count(expstats.MetricDescriptor{
Name: "grpc.lb.pick_first.disconnections",
Description: "EXPERIMENTAL. Number of times the selected subchannel becomes disconnected.",
Unit: "{disconnection}",
Labels: []string{"grpc.target"},
Default: false,
})
connectionAttemptsSucceededMetric = expstats.RegisterInt64Count(expstats.MetricDescriptor{
Name: "grpc.lb.pick_first.connection_attempts_succeeded",
Description: "EXPERIMENTAL. Number of successful connection attempts.",
Unit: "{attempt}",
Labels: []string{"grpc.target"},
Default: false,
})
connectionAttemptsFailedMetric = expstats.RegisterInt64Count(expstats.MetricDescriptor{
Name: "grpc.lb.pick_first.connection_attempts_failed",
Description: "EXPERIMENTAL. Number of failed connection attempts.",
Unit: "{attempt}",
Labels: []string{"grpc.target"},
Default: false,
})
)
var logger = grpclog.Component("pick-first-leaf-lb")

const (
// TODO: change to pick-first when this becomes the default pick_first policy.
Expand All @@ -101,11 +77,9 @@ const (

type pickfirstBuilder struct{}

func (pickfirstBuilder) Build(cc balancer.ClientConn, bo balancer.BuildOptions) balancer.Balancer {
func (pickfirstBuilder) Build(cc balancer.ClientConn, _ balancer.BuildOptions) balancer.Balancer {
b := &pickfirstBalancer{
cc: cc,
target: bo.Target.String(),
metricsRecorder: cc.MetricsRecorder(),
cc: cc,

subConns: resolver.NewAddressMapV2[*scData](),
state: connectivity.Connecting,
Expand Down Expand Up @@ -185,10 +159,8 @@ func (b *pickfirstBalancer) newSCData(addr resolver.Address) (*scData, error) {
type pickfirstBalancer struct {
// The following fields are initialized at build time and read-only after
// that and therefore do not need to be guarded by a mutex.
logger *internalgrpclog.PrefixLogger
cc balancer.ClientConn
target string
metricsRecorder expstats.MetricsRecorder // guaranteed to be non nil
logger *internalgrpclog.PrefixLogger
cc balancer.ClientConn

// The mutex is used to ensure synchronization of updates triggered
// from the idle picker and the already serialized resolver,
Expand Down Expand Up @@ -633,14 +605,11 @@ func (b *pickfirstBalancer) updateSubConnState(sd *scData, newState balancer.Sub
return
}

// Record a connection attempt when exiting CONNECTING.
if newState.ConnectivityState == connectivity.TransientFailure {
sd.connectionFailedInFirstPass = true
connectionAttemptsFailedMetric.Record(b.metricsRecorder, 1, b.target)
}

if newState.ConnectivityState == connectivity.Ready {
connectionAttemptsSucceededMetric.Record(b.metricsRecorder, 1, b.target)
b.shutdownRemainingLocked(sd)
if !b.addressList.seekTo(sd.addr) {
// This should not fail as we should have only one SubConn after
Expand Down Expand Up @@ -689,15 +658,6 @@ func (b *pickfirstBalancer) updateSubConnState(sd *scData, newState balancer.Sub
// the first address when the picker is used.
b.shutdownRemainingLocked(sd)
sd.effectiveState = newState.ConnectivityState
// READY SubConn interspliced in between CONNECTING and IDLE, need to
// account for that.
if oldState == connectivity.Connecting {
// A known issue (https://github.com/grpc/grpc-go/issues/7862)
// causes a race that prevents the READY state change notification.
// This works around it.
connectionAttemptsSucceededMetric.Record(b.metricsRecorder, 1, b.target)
}
disconnectionsMetric.Record(b.metricsRecorder, 1, b.target)
b.addressList.reset()
b.updateBalancerState(balancer.State{
ConnectivityState: connectivity.Idle,
Expand Down
40 changes: 4 additions & 36 deletions balancer/pickfirst/pickfirst_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2009,20 +2009,11 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TF_AfterEndOfList(t *testing.T) {
waitForMetric(ctx, t, tmr, "grpc.subchannel.connection_attempts_failed")

// Only connection attempt fails in this test.
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_succeeded", got, 0)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_failed", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0)
}
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_failed"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_failed", got, 1)
}
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)
Comment on lines -2024 to -2025

@easwars easwars Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Was this broken all this while then, @mbissa ?

@mbissa mbissa Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_succeeded", got, 0)
}
}

Expand Down Expand Up @@ -2075,16 +2066,6 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TriggerConnectionDelay(t *testing.T) {
testutils.AwaitState(ctx, t, cc, connectivity.Ready)
waitForMetric(ctx, t, tmr, "grpc.subchannel.connection_attempts_succeeded")
// Only connection attempt successes in this test.
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_succeeded", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_failed", got, 0)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0)
}

if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_succeeded", got, 1)
}
Expand Down Expand Up @@ -2153,9 +2134,6 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TF_ThenTimerFires(t *testing.T) {
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 {

@easwars easwars Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@mbissa mbissa Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to move the tests to different place which is more apt for subchannel and not keep them in pickfirst.

t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_failed", got, 1)
}
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_failed"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_failed", got, 1)
}
Expand All @@ -2179,11 +2157,8 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TF_ThenTimerFires(t *testing.T) {
if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.connection_attempts_succeeded", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.connection_attempts_succeeded", got, 1)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0)
if got, _ := tmr.Metric("grpc.subchannel.disconnections"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.subchannel.disconnections", got, 0)
}
}

Expand Down Expand Up @@ -2256,13 +2231,6 @@ func (s) TestPickFirstLeaf_HappyEyeballs_Ignore_Inflight_Cancellations(t *testin
}
}

// LB Metrics Check
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_succeeded"); got != 1 {
t.Errorf("Unexpected data for metric %v, got: %v, want: 1", "grpc.lb.pick_first.connection_attempts_succeeded", got)
}
if got, _ := tmr.Metric("grpc.lb.pick_first.connection_attempts_failed"); got != 0 {
t.Errorf("Unexpected data for metric %v, got: %v, want: 0", "grpc.lb.pick_first.connection_attempts_failed", got)
}
}

func (s) TestPickFirstLeaf_InterleavingIPV4Preferred(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions examples/features/opentelemetry/client/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ func main() {
// 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",
"grpc.subchannel.connection_attempts_succeeded",
"grpc.subchannel.connection_attempts_failed",
),
},
TraceOptions: oteltracing.TraceOptions{TracerProvider: traceProvider, TextMapPropagator: textMapPropagator},
Expand Down
8 changes: 0 additions & 8 deletions examples/features/opentelemetry/server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,6 @@ func main() {
so := opentelemetry.ServerOption(opentelemetry.Options{
MetricsOptions: opentelemetry.MetricsOptions{
MeterProvider: meterProvider,
// 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",
),
Comment on lines -76 to -83

@easwars easwars Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this different to the change in examples/features/opentelemetry/client/main.go?

@deepgolani4 deepgolani4 Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
TraceOptions: oteltracing.TraceOptions{TracerProvider: traceProvider, TextMapPropagator: textMapPropagator}})

Expand Down
4 changes: 2 additions & 2 deletions stats/opentelemetry/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ func ExampleOptions_addExperimentalMetrics() {
// 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",
"grpc.subchannel.connection_attempts_succeeded",
"grpc.subchannel.connection_attempts_failed",
),
},
}
Expand Down
Loading