Skip to content
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.

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

balancer: Move metrics recorder from BuildOptions to ClientConn #8027

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Jan 22, 2025

This PR changes the way LB policies obtain a metrics recorder from the channel. Earlier LB policies obtained it through the BuildOptions. Now the LB policies need to call the MetricsRecorder method on the ClientConn.

Why

When LB policies manage child policies, they need to correctly propagate the BuildOptions. If they fail to pass down the MetricsRecorder in the BuildOptions, it will read to nil pointer deferences when the child tries to record metrics. There is no way for gRPC to enforce LB polices propagate the MetricsRecorder. #8026 is open to enforce LB policies embed an existing ClientConn object. Using a method on ClientConn ensures that the MetricsRecorder can be obtained from the channel, unless explicitly intercepted by an LB policy. The MetricsRecorder field in the BuildOptions was added in the past 6 months, so we don't expect too many breakages in external code.

RELEASE NOTES:

  • balancer: Implementors of custom LB policies that record metrics must use the new MetricsRecorder method on Balancer.ClientConn instead of the removed Balancer.BuildOptions.MetricsRecorder field to obtain a metrics recorder.

@arjan-bal arjan-bal added Type: API Change Breaking API changes (experimental APIs only!) Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Jan 22, 2025
@arjan-bal arjan-bal added this to the 1.71 Release milestone Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.23%. Comparing base (3e27c17) to head (184db85).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8027      +/-   ##
==========================================
- Coverage   82.47%   82.23%   -0.24%     
==========================================
  Files         387      387              
  Lines       39037    39040       +3     
==========================================
- Hits        32194    32106      -88     
- Misses       5530     5602      +72     
- Partials     1313     1332      +19     
Files with missing lines Coverage Δ
balancer/balancer.go 96.29% <ø> (ø)
balancer/pickfirst/pickfirstleaf/pickfirstleaf.go 86.70% <100.00%> (-0.58%) ⬇️
balancer/rls/balancer.go 85.30% <100.00%> (-0.79%) ⬇️
balancer/weightedroundrobin/balancer.go 84.22% <100.00%> (+0.84%) ⬆️
balancer_wrapper.go 86.34% <100.00%> (-0.99%) ⬇️
internal/testutils/balancer.go 82.53% <100.00%> (+0.18%) ⬆️

... and 25 files with indirect coverage changes

balancer/balancer.go Outdated Show resolved Hide resolved
balancer/balancer.go Outdated Show resolved Hide resolved
balancer/balancer.go Outdated Show resolved Hide resolved
internal/testutils/test_metrics_recorder.go Outdated Show resolved Hide resolved
internal/internal.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned arjan-bal and unassigned dfawley Jan 23, 2025
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Jan 24, 2025
@dfawley dfawley assigned arjan-bal and unassigned dfawley Jan 24, 2025
@arjan-bal
Copy link
Contributor Author

Merged a PR to avoid breakages in grpc-gcp-go: GoogleCloudPlatform/grpc-gcp-go#91
Waiting on release of https://github.com/GoogleCloudPlatform/grpc-gcp-go.

@dfawley dfawley assigned arjan-bal and unassigned dfawley Jan 27, 2025
@arjan-bal arjan-bal merged commit f227ba9 into grpc:master Feb 6, 2025
15 checks passed
@arjan-bal arjan-bal deleted the move-metrics-recorder branch February 6, 2025 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: API Change Breaking API changes (experimental APIs only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants