feat(storage): add feature tracking for async stub#16179
Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a FeatureTracker utility to track and report client-side optimizations and configuration choices via the x-goog-storage-cpp-features header in both gRPC and REST clients. The reviewer recommends refactoring the duplicated logic for extracting and formatting the feature tracker header value into a shared helper function, FeatureTrackerHeaderValue, to comply with the repository's style guide against code duplication across three or more non-test files.
| // Evaluates client configuration options, creates a shared FeatureTracker | ||
| // initialized with configuration-driven feature flags (if any), and stores it | ||
| // into the Options list under FeatureTrackerOption. | ||
| Options SetupFeatureTracker(Options opts); |
There was a problem hiding this comment.
To adhere to the repository style guide (which prefers to factor out duplicated code if it appears 3 or more times in non-test files), we should introduce a helper function FeatureTrackerHeaderValue to extract and format the feature tracker header value. This avoids repeating the same extraction logic in configure_client_context.h, stub.cc, and storage_stub_factory.cc.
// Evaluates client configuration options, creates a shared FeatureTracker
// initialized with configuration-driven feature flags (if any), and stores it
// into the Options list under FeatureTrackerOption.
Options SetupFeatureTracker(Options opts);
// Returns the feature tracker header value if feature reports are enabled and
// a tracker is present in the options, and the header value is not empty.
// Otherwise, returns an empty string.
std::string FeatureTrackerHeaderValue(Options const& options);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 extract the feature tracker header value from options.
return opts;
}
std::string FeatureTrackerHeaderValue(Options const& options) {
if (options.has<EnableFeatureReportsOption>() &&
!options.get<EnableFeatureReportsOption>()) {
return {};
}
if (!options.has<FeatureTrackerOption>()) return {};
auto const& tracker = options.get<FeatureTrackerOption>();
if (!tracker) return {};
return tracker->HeaderValue();
}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<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 helper function FeatureTrackerHeaderValue to avoid duplicating the feature tracker extraction logic, adhering to the repository style guide.
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 helper function FeatureTrackerHeaderValue to avoid duplicating the feature tracker extraction logic, adhering to the repository style guide.
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)
| 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.
Use the helper function FeatureTrackerHeaderValue to avoid duplicating the feature tracker extraction logic, adhering to the repository style guide.
auto const val = storage::internal::FeatureTrackerHeaderValue(options);
if (!val.empty()) {
fixed_metadata.emplace(storage::internal::kFeatureTrackerHeaderName, val);
}References
- Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)
v-pratap
force-pushed
the
feat/storage-async-stub
branch
from
960cb1e to
970ecd9
Compare
1 participant
No description provided.