impl(storage): configure gRPC ClientContext using CurrentOptions#9453
impl(storage): configure gRPC ClientContext using CurrentOptions#9453dbolduc merged 1 commit intogoogleapis:mainfrom
CurrentOptions#9453Conversation
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9453 +/- ##
=======================================
Coverage 94.63% 94.63%
=======================================
Files 1486 1486
Lines 136679 136689 +10
=======================================
+ Hits 129342 129356 +14
+ Misses 7337 7333 -4
Continue to review full report at Codecov.
|
| EXPECT_CALL(mock, Call); | ||
|
|
||
| google::cloud::internal::OptionsSpan span( | ||
| Options{}.set<google::cloud::internal::GrpcSetupOption>( |
There was a problem hiding this comment.
A word of caution on what seems to be going on here.
An "internal option" implies to me that we're using thread locals to (conveniently?) plumb state through call layers.
I reckon such things would be better passed explicitly. Options should be reserved for truly optional things from the user. [Off soap box, and apologies if I'm mistaken.]
There was a problem hiding this comment.
Ack. The change is confusing on the surface. We are not using internal::GrpcSetupOption for state.
Basically, I want to test that current options (supplied by the user) will be used when setting up the grpc::ClientContext. But there are no such public options right now. So we test the one option that does anything: internal::GrpcSetupOption. In the future, I could see GrpcSetupOption being released, or see us adding other options like a MetadataOption. #4926 (comment)
Configure the
grpc::ClientContexts created by astorage::internal::GrpcClientusing theCurrentOptions().Note that, as of writing this, the only option that does anything is
google::cloud::internal::GrpcSetupOption. And we do not set this option. So we are only gaining a hook for debugging. And we are doing some extra map lookups on every client call. That is not obviously great.But, if we ever add an option for setting metadata or for setting a deadline we can have it take effect in
google::cloud::internal::ConfigureContext(..., Options). for gRPC. (See #4926 (comment) for how I see things playing out)This change is