Skip to content

feat(storage): Client Setup & REST feature tracking#16175

Draft
v-pratap wants to merge 2 commits into
googleapis:mainfrom
v-pratap:feat/storage-rest-feature-tracking
Draft

feat(storage): Client Setup & REST feature tracking#16175
v-pratap wants to merge 2 commits into
googleapis:mainfrom
v-pratap:feat/storage-rest-feature-tracking

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 mechanism to track and report client-side optimizations and configuration choices in the Google Cloud Storage C++ client via a custom x-goog-storage-cpp-features header. The feedback highlights two key improvements: first, optimizing EncodeFeatureTrackerBitmask by replacing std::vectorstd::uint8_t with std::string to leverage Small String Optimization (SSO) and avoid heap allocations on the hot path; second, correcting a test assertion in stub_test.cc where ElementsAre is incorrectly used on a string value, which would cause a compilation or matching failure.

Comment on lines +31 to +38
std::vector<std::uint8_t> bytes;
for (int i = 0; i < 4; ++i) {
auto b = static_cast<std::uint8_t>((mask >> (24 - i * 8)) & 0xFF);
if (!bytes.empty() || b != 0) {
bytes.push_back(b);
}
}
return Base64Encode(bytes);

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

Using std::vector<std::uint8_t> to accumulate the bytes for the bitmask causes a heap allocation on every call to EncodeFeatureTrackerBitmask (which is invoked on every REST request via AddHeaders).

Since the maximum size of the bitmask is 4 bytes, we can use std::string instead. Due to Small String Optimization (SSO), std::string will avoid heap allocation entirely, improving performance on the hot path.

  std::string bytes;
  for (int i = 0; i < 4; ++i) {
    auto b = static_cast<char>((mask >> (24 - i * 8)) & 0xFF);
    if (!bytes.empty() || b != 0) {
      bytes.push_back(b);
    }
  }
  return Base64Encode(bytes);

Comment on lines +1044 to +1045
Contains(Pair(kFeatureTrackerHeaderName,
ElementsAre(tracker->HeaderValue()))))))

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

Since RestRequest::headers() returns a collection of std::pair<std::string, std::string>, the value of the pair is a std::string, not a container. Using ElementsAre here is incorrect and will cause a compilation or matching failure. It should match the string directly.

              Contains(Pair(kFeatureTrackerHeaderName,
                            tracker->HeaderValue())))

@v-pratap v-pratap force-pushed the feat/storage-rest-feature-tracking branch from 3786379 to 6fea414 Compare June 16, 2026 18:28
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