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

Allow any registered gRPC load balancer to be used #8262

Merged
merged 4 commits into from
Aug 24, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 22, 2023

Description: gRPC-Go's balancer package includes a static registration mechanism and a way to inspect whether a balancer name is registered. We should use this mechanism instead of hard-coding an allowlist of balancer names.

Custom collector configurations may have additional balancers linked in, and we should allow them to be used.

Testing: existing cover this, pass

Documentation: Existing documentation refers to gRPC balancer package, does not mention any allowlist behavior.

@jmacd jmacd requested review from a team and Aneurysm9 August 22, 2023 17:25
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (3dad181) 90.18% compared to head (fb48525) 90.14%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8262      +/-   ##
==========================================
- Coverage   90.18%   90.14%   -0.04%     
==========================================
  Files         302      302              
  Lines       15766    15771       +5     
==========================================
- Hits        14218    14217       -1     
- Misses       1254     1259       +5     
- Partials      294      295       +1     
Files Changed Coverage Δ
config/configgrpc/configgrpc.go 91.44% <100.00%> (-0.16%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

@bogdandrutu
Copy link
Member

@jmacd please add a changelog about this enhancement

@codeboten codeboten closed this Aug 22, 2023
@codeboten codeboten reopened this Aug 22, 2023
@codeboten
Copy link
Contributor

Apologies, i accidentally closed the PR on github from my phone 😬

@jmacd
Copy link
Contributor Author

jmacd commented Aug 23, 2023

I made a changelog, but there's no issue filed and it makes me think this PR doesn't really need a changelog entry. Do we need an issue?

@jmacd
Copy link
Contributor Author

jmacd commented Aug 23, 2023

I'll file an issue stating that changelog issues shouldn't require issues, though :)

@jmacd jmacd added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 23, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

You can use the PR number or an issue number @jmacd

@jmacd jmacd removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 23, 2023
@codeboten
Copy link
Contributor

Looks like one linting error to address:

Error: configgrpc_test.go:42:34: unused-parameter: parameter 'cc' seems to be unused, consider removing or renaming it as _ (revive)
func (testBalancerBuilder) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer {

@codeboten codeboten merged commit 2fe11f5 into open-telemetry:main Aug 24, 2023
30 checks passed
@codeboten codeboten added this to the next release milestone Aug 24, 2023
@bogdandrutu
Copy link
Member

@jmacd why do you hate linters?

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.

3 participants