Skip to content

rls, grpc: inherit parent channel stats handlers and interceptors on the RLS control channel - #9362

Closed
sushanb wants to merge 2 commits into
grpc:masterfrom
sushanb:fix/rls-control-channel-inherit-parent-telemetry
Closed

rls, grpc: inherit parent channel stats handlers and interceptors on the RLS control channel#9362
sushanb wants to merge 2 commits into
grpc:masterfrom
sushanb:fix/rls-control-channel-inherit-parent-telemetry

Conversation

@sushanb

@sushanb sushanb commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Stacked on #9363. This PR depends on #9363 (adds Unwrap() to the balancer.ClientConn-embedding wrappers). Please review #9363 first. Once it merges, this PR will be rebased onto master and its diff will shrink to just the changes below.

Summary

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.

Approach

Introduce an internal hook internal.NewChannelForBalancer (populated in clientconn.go init, mirroring SubscribeToConnectivityStateChanges and other existing internal-hook patterns). It 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 via the Unwrap() balancer.ClientConn contract that every embedding wrapper implements after #9363. A seen set guards against cycles. No reflection — all wrapper hops are explicit.

RLS now dials via this hook (balancer/rls/control_channel.go) instead of grpc.NewClient; rlsBalancer passes its b.cc through.

No public API change

No new fields on balancer.BuildOptions, no new methods on balancer.ClientConn — the mechanism is entirely internal, keeping the API surface stable.

Verification

  • Existing test suite passes: ./balancer/..., ./stats/opentelemetry/..., ./internal/balancer/..., ./internal/balancergroup/..., root grpc.
  • New regression test TestRLSControlChannelAttemptDurationMetric in balancer/rls/metrics_test.go asserts a grpc.client.attempt.duration data point is recorded with grpc.method=grpc.lookup.v1.RouteLookupService/RouteLookup after a real dial with opentelemetry.DialOption triggers a RouteLookup via the RLS balancer.
  • End-to-end verified against a real Bigtable DirectPath workload (~110s of traffic): RouteLookup attempts to dns:///bigtablerls.googleapis.com show up in grpc.client.attempt.duration with method, target, and status labels populated correctly.

Test plan

  • Confirm go test ./... passes in CI
  • Confirm no regressions in xDS integration tests

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.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.72093% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.43%. Comparing base (9d1988d) to head (069665d).

Files with missing lines Patch % Lines
clientconn.go 93.93% 2 Missing ⚠️
balancer/grpclb/grpclb_util.go 0.00% 1 Missing ⚠️
balancer/ringhash/ringhash.go 0.00% 1 Missing ⚠️
internal/xds/balancer/clusterimpl/clusterimpl.go 0.00% 1 Missing ⚠️
internal/xds/balancer/outlierdetection/balancer.go 0.00% 1 Missing ⚠️
...ternal/xds/balancer/priority/ignore_resolve_now.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9362      +/-   ##
==========================================
- Coverage   87.45%   87.43%   -0.02%     
==========================================
  Files         425      425              
  Lines       30297    30338      +41     
==========================================
+ Hits        26496    26527      +31     
- Misses       3801     3811      +10     
Files with missing lines Coverage Δ
balancer/rls/balancer.go 90.63% <100.00%> (ø)
balancer/rls/control_channel.go 90.27% <100.00%> (+0.13%) ⬆️
internal/balancer/gracefulswitch/gracefulswitch.go 92.76% <100.00%> (+2.69%) ⬆️
internal/balancergroup/balancergroup.go 88.94% <100.00%> (-1.00%) ⬇️
internal/internal.go 66.66% <ø> (ø)
balancer/grpclb/grpclb_util.go 81.35% <0.00%> (-4.86%) ⬇️
balancer/ringhash/ringhash.go 95.55% <0.00%> (-0.72%) ⬇️
internal/xds/balancer/clusterimpl/clusterimpl.go 92.13% <0.00%> (-0.35%) ⬇️
internal/xds/balancer/outlierdetection/balancer.go 92.03% <0.00%> (-0.53%) ⬇️
...ternal/xds/balancer/priority/ignore_resolve_now.go 87.50% <0.00%> (-12.50%) ⬇️
... and 1 more

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…pers

Several types in the balancer tree wrap a balancer.ClientConn by
anonymously embedding it. Add an Unwrap() balancer.ClientConn method to
each of them so that gRPC-internal callers can walk up the wrapping
chain to reach the underlying parent ClientConn. This is the
mirror-image contract of what errors.Unwrap provides for error trees.

The method is a one-liner returning the embedded field, and doesn't
change any existing behavior. It exists so a follow-up change can rely
on it to inherit parent-channel telemetry (stats handlers and
interceptors) onto balancer-owned control channels (e.g. the RLS
control channel) without resorting to reflection over private fields.

Wrappers touched:
  internal/balancer/gracefulswitch.balancerWrapper
  internal/balancergroup.subBalancerWrapper
  internal/xds/balancer/clusterimpl.clusterImplBalancer
  internal/xds/balancer/outlierdetection.outlierDetectionBalancer
  internal/xds/balancer/priority.ignoreResolveNowClientConn
  balancer/grpclb.lbCacheClientConn
  balancer/ringhash.ringhashBalancer

RELEASE NOTES: none
…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.
@sushanb
sushanb force-pushed the fix/rls-control-channel-inherit-parent-telemetry branch from c81f28e to 069665d Compare August 25, 2026 03:45
@sushanb
sushanb marked this pull request as draft August 25, 2026 03:56
@sushanb

sushanb commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Converting to draft while I re-implement the fix to conform to gRFC A110 (Child Channel Options). This PR's approach (auto-inheriting the parent channel's stats handlers + interceptors via an internal walk) violates A110's explicit rule that O_child be opaque to P, and doesn't add the public grpc.WithChildChannelOptions/balancer.BuildOptions.ChildChannelOptions API that A110 mandates. Replacement PR incoming — will link here.

@eshitachandwani

Copy link
Copy Markdown
Member

We generally do not prefer keeping draft PRs. Please re-open once the implementation is done and ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants