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

Added round_robin balancer as an option to gRPC client settings #1353

Merged
merged 11 commits into from
Jul 15, 2020

Conversation

RashmiRam
Copy link
Contributor

Description:
Allow gRPC client settings like balancerName to be configured via config file.

Link to tracking Issue:
#1041

Testing:
No new tests are added. Added this option also in TestAllGrpcClientSettings test

Documentation:
Updated the README.md of jaegerexporter, opencensusexporter, otlpexporter to reflect the newly added option.

Closes #1041

@RashmiRam
Copy link
Contributor Author

RashmiRam commented Jul 14, 2020

There are only 2 available balancers pick_first and round_robin. Default is pick_first. Since user of otel-col don't have any ways to create a custom Balancer to be used in lb policy of grpc client settings. We could make it simple with adding one option such as useRoundRobin to set the policy as round_robin if enabled. Otherwise, we might have to validate the input balancer name as invalid name will result in error.

@bogdandrutu Let me know your thoughts too.


// Sets the balancer as RoundRobin in grpclb_policy to discover the servers. Default is pick_first
// https://github.com/grpc/grpc-go/blob/master/examples/features/load_balancing/README.md
useRoundRobin bool `mapstructure:"useRoundRobin"`
Copy link
Member

Choose a reason for hiding this comment

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

I would use a string here instead of book, just in case other strategies will be supported. Same behavior, if not set use the first pick. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it from bool to string and validated the same as setting invalid balancer name causes panic.

@bogdandrutu
Copy link
Member

@RashmiRam I read your comment after I added mine. I still think having a string is better because gRPC may support other policies in the future and the problem with the boolean will hit us even if the user cannot create custom lb policies.

…w balancers in future

Setting invalid balancer is panicking. Hence validated the same & thrown an error
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #1353 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1353   +/-   ##
=======================================
  Coverage   90.14%   90.14%           
=======================================
  Files         218      218           
  Lines       15256    15266   +10     
=======================================
+ Hits        13752    13762   +10     
  Misses       1095     1095           
  Partials      409      409           
Impacted Files Coverage Δ
config/configgrpc/configgrpc.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 86.04% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b964ca4...cd13a42. Read the comment docs.

@RashmiRam
Copy link
Contributor Author

The lint error seen in build failure is because of the usage of deprecated grpc.WithBalancerName. Recommendation is to use grpc.WithDefaultServiceConfig. There is an open issue about the usage of the same grpc/grpc-go#3003.

How do we go about this? Shall we make use of the grpc.WithDefaultServiceConfig and revisit this later?

@bogdandrutu
Copy link
Member

I would go with that ugly API for the moment.

return opts, nil
}

func validateBalancerName(balancerName string) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we explicitly check here for the right values. I know we will need work when new format is accepted but that way we control our backwards compatible story

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I couldn't get what you mean by backwards compatible story? Do you mean the API change for setting the balancerName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I ended up removing the other call to grpc.WithBalancerName in validateBalancerName to fix the lint errors.

@bogdandrutu bogdandrutu merged commit a509d72 into open-telemetry:master Jul 15, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
open-telemetry#1353)

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
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.

Support for configuring more gRPC client settings
2 participants