feat(storage): Synchronous gRPC Interceptor #16176
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a FeatureTracker to monitor and report client-side optimizations and configuration choices via the x-goog-storage-cpp-features header. The code reviewer identified that statically adding the tracker during stub creation in StorageMetadata is redundant and can lead to stale headers, recommending its removal along with the associated test. Additionally, to comply with the repository's style guide, the reviewer suggested factoring out duplicated logic for retrieving the tracker's header value into a centralized helper function, FeatureTrackerHeaderValue, to be used across both gRPC and REST paths.
| std::multimap<std::string, std::string> fixed_metadata; | ||
| bool const enable_reports = | ||
| !options.has<storage::EnableFeatureReportsOption>() || | ||
| options.get<storage::EnableFeatureReportsOption>(); | ||
| if (enable_reports && | ||
| options.has<storage::internal::FeatureTrackerOption>()) { | ||
| auto const& tracker = | ||
| options.get<storage::internal::FeatureTrackerOption>(); | ||
| if (tracker) { | ||
| auto const val = tracker->HeaderValue(); | ||
| if (!val.empty()) { | ||
| fixed_metadata.emplace(storage::internal::kFeatureTrackerHeaderName, | ||
| val); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Adding the feature tracker header to fixed_metadata in StorageMetadata captures a static snapshot of the tracker's value at stub creation time. Since ApplyQueryParameters (for unary RPCs) and OpenObject::CreateRpc (for streaming RPCs) already dynamically add the up-to-date header value to the ClientContext for each request, having it in StorageMetadata is redundant and leads to duplicate x-goog-storage-cpp-features headers on the wire. Furthermore, the static value in StorageMetadata will not reflect any operation-driven features registered after stub creation, potentially sending stale/conflicting information. We should remove this block entirely.
std::multimap<std::string, std::string> fixed_metadata;| Options SetupFeatureTracker(Options opts); | ||
|
|
There was a problem hiding this comment.
To adhere to the repository style guide regarding factoring out duplicated code (which appears 3 or more times in non-test files), we should introduce a helper function FeatureTrackerHeaderValue to centralize the logic of checking EnableFeatureReportsOption and retrieving the header value.
Options SetupFeatureTracker(Options opts);
std::string FeatureTrackerHeaderValue(Options const& opts);References
- prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)
| return opts; | ||
| } | ||
|
|
There was a problem hiding this comment.
Implement the helper function FeatureTrackerHeaderValue to centralize the logic of checking if feature reporting is enabled and retrieving the header value from the tracker.
return opts;
}
std::string FeatureTrackerHeaderValue(Options const& opts) {
if (opts.has<EnableFeatureReportsOption>() &&
!opts.get<EnableFeatureReportsOption>()) {
return {};
}
if (!opts.has<FeatureTrackerOption>()) return {};
auto const& tracker = opts.get<FeatureTrackerOption>();
if (!tracker) return {};
return tracker->HeaderValue();
}| bool const enable_reports = | ||
| !options.has<storage::EnableFeatureReportsOption>() || | ||
| options.get<storage::EnableFeatureReportsOption>(); | ||
| if (enable_reports && | ||
| options.has<storage::internal::FeatureTrackerOption>()) { | ||
| auto const& tracker = | ||
| options.get<storage::internal::FeatureTrackerOption>(); | ||
| if (tracker) { | ||
| auto const val = tracker->HeaderValue(); | ||
| if (!val.empty()) { | ||
| ctx.AddMetadata(storage::internal::kFeatureTrackerHeaderName, val); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the centralized FeatureTrackerHeaderValue helper function to simplify the code and avoid duplication.
auto const val = storage::internal::FeatureTrackerHeaderValue(options);
if (!val.empty()) {
ctx.AddMetadata(storage::internal::kFeatureTrackerHeaderName, val);
}References
- prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)
| bool const enable_reports = | ||
| !options.has<EnableFeatureReportsOption>() || | ||
| options.get<EnableFeatureReportsOption>(); | ||
| if (enable_reports && options.has<FeatureTrackerOption>()) { | ||
| auto const& tracker = options.get<FeatureTrackerOption>(); | ||
| if (tracker) { | ||
| auto const val = tracker->HeaderValue(); | ||
| if (!val.empty()) { | ||
| builder.AddHeader(kFeatureTrackerHeaderName, val); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the centralized FeatureTrackerHeaderValue helper function to simplify the code and avoid duplication.
auto const val = FeatureTrackerHeaderValue(options);
if (!val.empty()) {
builder.AddHeader(kFeatureTrackerHeaderName, val);
}References
- prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)
| TEST_F(StorageStubFactory, FeatureTrackerInStorageMetadata) { | ||
| auto tracker = std::make_shared<storage::internal::FeatureTracker>(); | ||
| tracker->RegisterFeature(storage::internal::TrackedFeature::kPCU); | ||
|
|
||
| MockFactory factory; | ||
| EXPECT_CALL(factory, Call) | ||
| .WillOnce([&](std::shared_ptr<grpc::Channel> const&) { | ||
| auto mock = std::make_shared<MockStorageStub>(); | ||
| EXPECT_CALL(*mock, DeleteBucket) | ||
| .WillOnce([&](grpc::ClientContext& context, Options const&, | ||
| google::storage::v2::DeleteBucketRequest const&) { | ||
| ValidateMetadataFixture fixture; | ||
| auto metadata = fixture.GetMetadata(context); | ||
| EXPECT_THAT(metadata, | ||
| Contains(Pair(storage::internal::kFeatureTrackerHeaderName, | ||
| tracker->HeaderValue()))); | ||
| return internal::AbortedError("fail"); | ||
| }); | ||
| return mock; | ||
| }); | ||
|
|
||
| internal::AutomaticallyCreatedBackgroundThreads pool; | ||
| auto stub = CreateTestStub( | ||
| pool.cq(), factory.AsStdFunction(), | ||
| Options{} | ||
| .set<GrpcNumChannelsOption>(1) | ||
| .set<storage::internal::FeatureTrackerOption>(tracker)); | ||
| grpc::ClientContext context; | ||
| (void)stub->DeleteBucket(context, Options{}, {}); | ||
| } |
a65e372 to
c916a33
Compare
c916a33 to
f790d95
Compare
No description provided.