feat(common): QuotaUserOption for gRPC-based libs
#13933
Conversation
google/cloud/grpc_options.cc
Outdated
| @@ -31,6 +32,12 @@ void ConfigureContext(grpc::ClientContext& context, Options const& opts) { | |||
| context.set_compression_algorithm( | |||
| opts.get<GrpcCompressionAlgorithmOption>()); | |||
| } | |||
| if (opts.has<UserIpOption>() && !opts.has<QuotaUserOption>()) { | |||
There was a problem hiding this comment.
Putting this in the metadata decorator would be more consistent:
google-cloud-cpp/google/cloud/storage/internal/storage_metadata_decorator.cc
Lines 879 to 887 in 4db2434
There was a problem hiding this comment.
Hmm... if anything, the metadata decorator should not set these common options and just call a common function to do it.
There was a problem hiding this comment.
if anything, the metadata decorator should not set these common options and just call a common function to do it.
Sure, that will save us N lines of code per service.
I sort of think that the base stub should call ConfigureContext(), instead of the retry loops calling it.
Part of the reason why I would steer you away from ConfigureContext() is that I don't trust its coverage. For example, the polling loops do not use it:
There was a problem hiding this comment.
Let's merge #13935 and then I can make the changes to that function, would that work?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13933 +/- ##
=======================================
Coverage 93.12% 93.12%
=======================================
Files 2181 2181
Lines 185840 185870 +30
=======================================
+ Hits 173065 173098 +33
+ Misses 12775 12772 -3 ☔ View full report in Codecov by Sentry. |
coryan
force-pushed
the
feat-common-quota-user-for-gRPC
branch
from
6d428bb
to
404cd19
Compare
google/cloud/common_options.h
Outdated
| * | ||
| * This can be used to separate quota usage by source IP address. | ||
| * | ||
| * @deprecated prefer using `google::cloud::QuotaUser`. |
There was a problem hiding this comment.
s/QuotaUser/QuotaUserOption/?
| }; | ||
|
|
||
| /** | ||
| * Configure the UserIp system parameter. |
There was a problem hiding this comment.
Was "system parameter" supposed to be a link?
Fixes #13928
This change is