Skip to content

feat(storage): add feature tracking for async stub#16179

Draft
v-pratap wants to merge 4 commits into
googleapis:mainfrom
v-pratap:feat/storage-async-stub
Draft

feat(storage): add feature tracking for async stub#16179
v-pratap wants to merge 4 commits into
googleapis:mainfrom
v-pratap:feat/storage-async-stub

Conversation

@v-pratap

Copy link
Copy Markdown
Contributor

No description provided.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Jun 16, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +77 to +80
// 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)

Comment on lines +51 to +52
return opts;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)

Comment on lines +76 to +89
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);
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)

Comment on lines +122 to +133
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);
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)

Comment on lines +92 to +106
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);
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)

@v-pratap v-pratap force-pushed the feat/storage-async-stub branch from 960cb1e to 970ecd9 Compare June 16, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant