Skip to content

Commit 069665d

Browse files
committed
rls, grpc: inherit parent channel stats handlers and interceptors on the RLS control channel
RouteLookup RPCs issued on the RLS balancer's control channel were invisible to any stats handler configured on the parent ClientConn (e.g. OpenTelemetry's grpc.client.attempt.duration histogram), because the control channel was created via a bare grpc.NewClient call that had no way to see the parent channel's stats handlers or client interceptors. Introduce an internal hook, NewChannelForBalancer, that creates a new ClientConn while inheriting the parent's stats handlers and unary/stream interceptors (single and chained). The parent is reached by walking the balancer.ClientConn wrapping chain using the Unwrap() balancer.ClientConn contract that every wrapper in the tree now implements (added in a prior change). A seen-set guards against cycles. RLS now dials via this hook instead of grpc.NewClient. Verified end-to-end against a real Bigtable DirectPath workload: RouteLookup attempts to dns:///bigtablerls.googleapis.com show up in grpc.client.attempt.duration with method, target, and status labels populated correctly. Regression test added in balancer/rls/metrics_test.go. RELEASE NOTES: * rls: RouteLookup RPCs issued on the RLS balancer's control channel now inherit stats handlers and client interceptors configured on the parent ClientConn, so per-attempt telemetry (e.g. grpc.client.attempt.duration) covers RouteLookup RPCs.
1 parent 3e994fa commit 069665d

6 files changed

Lines changed: 146 additions & 9 deletions

File tree

balancer/rls/balancer.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ func (b *rlsBalancer) handleControlChannelUpdate(newCfg *lbConfig) {
367367
backToReadyFn := func() {
368368
b.updateCh.Put(controlChannelReady{})
369369
}
370-
ctrlCh, err := newControlChannel(newCfg.lookupService, newCfg.controlChannelServiceConfig, newCfg.lookupServiceTimeout, b.bopts, backToReadyFn)
370+
ctrlCh, err := newControlChannel(b.cc, newCfg.lookupService, newCfg.controlChannelServiceConfig, newCfg.lookupServiceTimeout, b.bopts, backToReadyFn)
371371
if err != nil {
372372
// This is very uncommon and usually represents a non-transient error.
373373
// There is not much we can do here other than wait for another update

balancer/rls/control_channel.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,14 @@ type controlChannel struct {
7272
// newControlChannel creates a controlChannel to rlsServerName and uses
7373
// serviceConfig, if non-empty, as the default service config for the underlying
7474
// gRPC channel.
75-
func newControlChannel(rlsServerName, serviceConfig string, rpcTimeout time.Duration, bOpts balancer.BuildOptions, backToReadyFunc func()) (*controlChannel, error) {
75+
//
76+
// parentCC is the balancer.ClientConn passed to Balancer.Build. It is used to
77+
// inherit the parent channel's stats handlers and client interceptors so that
78+
// per-attempt telemetry configured on the parent channel also covers
79+
// RouteLookup RPCs. A nil parentCC is tolerated (control channel is created
80+
// without inheriting parent telemetry) for tests that construct the control
81+
// channel directly.
82+
func newControlChannel(parentCC balancer.ClientConn, rlsServerName, serviceConfig string, rpcTimeout time.Duration, bOpts balancer.BuildOptions, backToReadyFunc func()) (*controlChannel, error) {
7683
ctrlCh := &controlChannel{
7784
rpcTimeout: rpcTimeout,
7885
backToReadyFunc: backToReadyFunc,
@@ -84,7 +91,8 @@ func newControlChannel(rlsServerName, serviceConfig string, rpcTimeout time.Dura
8491
if err != nil {
8592
return nil, err
8693
}
87-
ctrlCh.cc, err = grpc.NewClient(rlsServerName, dopts...)
94+
newChan := internal.NewChannelForBalancer.(func(balancer.ClientConn, string, ...grpc.DialOption) (*grpc.ClientConn, error))
95+
ctrlCh.cc, err = newChan(parentCC, rlsServerName, dopts...)
8896
if err != nil {
8997
return nil, err
9098
}

balancer/rls/control_channel_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (s) TestControlChannelThrottled(t *testing.T) {
5151
overrideAdaptiveThrottler(t, alwaysThrottlingThrottler())
5252

5353
// Create a control channel to the fake RLS server.
54-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{}, nil)
54+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{}, nil)
5555
if err != nil {
5656
t.Fatalf("Failed to create control channel to RLS server: %v", err)
5757
}
@@ -79,7 +79,7 @@ func (s) TestLookupFailure(t *testing.T) {
7979
})
8080

8181
// Create a control channel to the fake RLS server.
82-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{}, nil)
82+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{}, nil)
8383
if err != nil {
8484
t.Fatalf("Failed to create control channel to RLS server: %v", err)
8585
}
@@ -118,7 +118,7 @@ func (s) TestLookupDeadlineExceeded(t *testing.T) {
118118
overrideAdaptiveThrottler(t, neverThrottlingThrottler())
119119

120120
// Create a control channel with a small deadline.
121-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestShortTimeout, balancer.BuildOptions{}, nil)
121+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestShortTimeout, balancer.BuildOptions{}, nil)
122122
if err != nil {
123123
t.Fatalf("Failed to create control channel to RLS server: %v", err)
124124
}
@@ -272,7 +272,7 @@ func testControlChannelCredsSuccess(t *testing.T, sopts []grpc.ServerOption, bop
272272
})
273273

274274
// Create a control channel to the fake server.
275-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestTimeout, bopts, nil)
275+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestTimeout, bopts, nil)
276276
if err != nil {
277277
t.Fatalf("Failed to create control channel to RLS server: %v", err)
278278
}
@@ -360,7 +360,7 @@ func testControlChannelCredsFailure(t *testing.T, sopts []grpc.ServerOption, bop
360360
overrideAdaptiveThrottler(t, neverThrottlingThrottler())
361361

362362
// Create the control channel to the fake server.
363-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestTimeout, bopts, nil)
363+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestTimeout, bopts, nil)
364364
if err != nil {
365365
t.Fatalf("Failed to create control channel to RLS server: %v", err)
366366
}
@@ -457,7 +457,7 @@ func (s) TestNewControlChannelUnsupportedCredsBundle(t *testing.T) {
457457
rlsServer, _ := rlstest.SetupFakeRLSServer(t, nil)
458458

459459
// Create the control channel to the fake server.
460-
ctrlCh, err := newControlChannel(rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{CredsBundle: &unsupportedCredsBundle{}}, nil)
460+
ctrlCh, err := newControlChannel(nil, rlsServer.Address, "", defaultTestTimeout, balancer.BuildOptions{CredsBundle: &unsupportedCredsBundle{}}, nil)
461461
if err == nil {
462462
ctrlCh.close()
463463
t.Fatal("newControlChannel succeeded when expected to fail")

balancer/rls/metrics_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,3 +373,63 @@ func (s) TestRLSFailedRPCMetric(t *testing.T) {
373373
}
374374
}
375375
}
376+
377+
// TestRLSControlChannelAttemptDurationMetric verifies that per-attempt
378+
// telemetry configured on the parent channel (grpc.client.attempt.duration)
379+
// also covers the RouteLookup RPC issued by the RLS balancer's control
380+
// channel. Regression test for the plumbing that inherits parent stats
381+
// handlers and interceptors onto balancer-owned control channels.
382+
func (s) TestRLSControlChannelAttemptDurationMetric(t *testing.T) {
383+
rlsServer, _ := rlstest.SetupFakeRLSServer(t, nil)
384+
rlsConfig := buildBasicRLSConfigWithChildPolicy(t, t.Name(), rlsServer.Address)
385+
backend := &stubserver.StubServer{
386+
EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) {
387+
return &testpb.Empty{}, nil
388+
},
389+
}
390+
if err := backend.StartServer(); err != nil {
391+
t.Fatalf("Failed to start backend: %v", err)
392+
}
393+
defer backend.Stop()
394+
rlsConfig.RouteLookupConfig.DefaultTarget = backend.Address
395+
396+
r := startManualResolverWithConfig(t, rlsConfig)
397+
reader := metric.NewManualReader()
398+
provider := metric.NewMeterProvider(metric.WithReader(reader))
399+
mo := opentelemetry.MetricsOptions{MeterProvider: provider}
400+
cc, err := grpc.NewClient(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), opentelemetry.DialOption(opentelemetry.Options{MetricsOptions: mo}))
401+
if err != nil {
402+
t.Fatalf("Failed to dial local test server: %v", err)
403+
}
404+
defer cc.Close()
405+
406+
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
407+
defer cancel()
408+
if _, err := testgrpc.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{}); err != nil {
409+
t.Fatalf("client.EmptyCall failed: %v", err)
410+
}
411+
412+
const rlsMethod = "grpc.lookup.v1.RouteLookupService/RouteLookup"
413+
md, ok := metricsDataFromReader(ctx, reader)["grpc.client.attempt.duration"]
414+
if !ok {
415+
t.Fatal("grpc.client.attempt.duration metric not recorded")
416+
}
417+
hist, ok := md.Data.(metricdata.Histogram[float64])
418+
if !ok {
419+
t.Fatalf("grpc.client.attempt.duration has unexpected data type %T, want Histogram[float64]", md.Data)
420+
}
421+
for _, dp := range hist.DataPoints {
422+
if v, ok := dp.Attributes.Value("grpc.method"); ok && v.AsString() == rlsMethod {
423+
return
424+
}
425+
}
426+
t.Fatalf("grpc.client.attempt.duration did not record a data point for method %q; got attribute sets %v", rlsMethod, attributeSets(hist.DataPoints))
427+
}
428+
429+
func attributeSets(dps []metricdata.HistogramDataPoint[float64]) []attribute.Set {
430+
out := make([]attribute.Set, 0, len(dps))
431+
for _, dp := range dps {
432+
out = append(out, dp.Attributes)
433+
}
434+
return out
435+
}

clientconn.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,67 @@ func init() {
790790
internal.ExitIdleModeForTesting = func(cc *ClientConn) {
791791
cc.idlenessMgr.ExitIdleMode()
792792
}
793+
internal.NewChannelForBalancer = newChannelForBalancer
794+
}
795+
796+
// newChannelForBalancer creates a new ClientConn that inherits parent's stats
797+
// handlers and client interceptors. Used by LB policies that open their own
798+
// control-plane channels (e.g. RLS) so per-attempt telemetry configured on the
799+
// parent channel also covers those control-plane RPCs. Inherited options are
800+
// prepended to opts, so callers may still override them by passing their own
801+
// stats handlers or interceptors in opts.
802+
//
803+
// The balancer.ClientConn handed to Balancer.Build is often wrapped several
804+
// layers deep in xDS/DirectPath configs (gracefulswitch, balancer group,
805+
// cluster_impl, outlier_detection, ...). Every wrapping layer exposes its
806+
// delegate via Unwrap() balancer.ClientConn (see the same-named methods on
807+
// those types); walk that chain until we find the underlying
808+
// *ccBalancerWrapper that holds the parent *ClientConn. If none is found,
809+
// fall back to a plain NewClient so the caller still gets a working channel.
810+
func newChannelForBalancer(parent balancer.ClientConn, target string, opts ...DialOption) (*ClientConn, error) {
811+
type unwrapper interface{ Unwrap() balancer.ClientConn }
812+
var ccb *ccBalancerWrapper
813+
seen := make(map[balancer.ClientConn]struct{})
814+
for cur := parent; cur != nil; {
815+
if c, ok := cur.(*ccBalancerWrapper); ok {
816+
ccb = c
817+
break
818+
}
819+
if _, dup := seen[cur]; dup {
820+
break
821+
}
822+
seen[cur] = struct{}{}
823+
u, ok := cur.(unwrapper)
824+
if !ok {
825+
break
826+
}
827+
next := u.Unwrap()
828+
if next == nil || next == cur {
829+
break
830+
}
831+
cur = next
832+
}
833+
if ccb == nil || ccb.cc == nil {
834+
return NewClient(target, opts...)
835+
}
836+
pd := ccb.cc.dopts
837+
inherited := make([]DialOption, 0, len(pd.copts.StatsHandlers)+4)
838+
for _, sh := range pd.copts.StatsHandlers {
839+
inherited = append(inherited, WithStatsHandler(sh))
840+
}
841+
if pd.unaryInt != nil {
842+
inherited = append(inherited, WithUnaryInterceptor(pd.unaryInt))
843+
}
844+
if len(pd.chainUnaryInts) > 0 {
845+
inherited = append(inherited, WithChainUnaryInterceptor(pd.chainUnaryInts...))
846+
}
847+
if pd.streamInt != nil {
848+
inherited = append(inherited, WithStreamInterceptor(pd.streamInt))
849+
}
850+
if len(pd.chainStreamInts) > 0 {
851+
inherited = append(inherited, WithChainStreamInterceptor(pd.chainStreamInts...))
852+
}
853+
return NewClient(target, append(inherited, opts...)...)
793854
}
794855

795856
func (cc *ClientConn) maybeApplyDefaultServiceConfig() {

internal/internal.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,14 @@ var (
143143
// provided grpc.ClientConn.
144144
SubscribeToConnectivityStateChanges any // func(*grpc.ClientConn, grpcsync.Subscriber)
145145

146+
// NewChannelForBalancer creates a new grpc.ClientConn that inherits the
147+
// parent ClientConn's stats handlers and client interceptors. It is meant
148+
// to be used by LB policies that open their own control-plane channels
149+
// (for example, RLS) so that per-attempt telemetry configured on the
150+
// parent channel also covers those control-plane RPCs. The parent argument
151+
// must be the balancer.ClientConn that gRPC passed to Balancer.Build.
152+
NewChannelForBalancer any // func(parent balancer.ClientConn, target string, opts ...grpc.DialOption) (*grpc.ClientConn, error)
153+
146154
// NewXDSResolverWithConfigForTesting creates a new xds resolver builder using
147155
// the provided xds bootstrap config instead of the global configuration from
148156
// the supported environment variables. The resolver.Builder is meant to be

0 commit comments

Comments
 (0)