Repository navigation
feat(storage): add client feature tracking header - #16591
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 client-side feature tracking to report active features, such as multi-stream optimization and read hedging, to the storage service via a new x-goog-storage-cpp-features header. The features are encoded as a 4-byte big-endian bitmask using URL-safe base64. The review feedback highlights several violations of the repository style guide regarding the use of auto, specifically requesting explicit types for primitives and when auto obscures function return types or loop variables.
| auto const enable_hedging = | ||
| current->get<storage_experimental::EnableReadHedgingOption>(); | ||
| auto const delay = current->get<storage_experimental::ReadHedgeDelayOption>(); | ||
| auto const max_hedges = | ||
| current->get<storage_experimental::MaxReadHedgesOption>(); | ||
| auto const max_buffer = | ||
| current->get<storage_experimental::MaximumHedgeBufferOption>(); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used for primitive or scalar types (such as bool, int, std::size_t). Please use explicit types instead.
bool const enable_hedging =
current->get<storage_experimental::EnableReadHedgingOption>();
auto const delay = current->get<storage_experimental::ReadHedgeDelayOption>();
int const max_hedges =
current->get<storage_experimental::MaxReadHedgesOption>();
std::size_t const max_buffer =
current->get<storage_experimental::MaximumHedgeBufferOption>();References
- Disallow
autofor Primitives: Require explicit numeric and scalar types (std::size_t,std::int64_t,bool, etc.) rather than deducing them from literals. (link)
| features |= storage_internal::FeatureBit( | ||
| storage_internal::TrackedFeature::kReadHedging); | ||
| } | ||
| auto const features_value = storage_internal::EncodeFeatureMask(features); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides function return types. Please use std::string explicitly.
std::string const features_value = storage_internal::EncodeFeatureMask(features);References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| auto const& headers = context.headers(); | ||
| auto const l = headers.find(name); | ||
| if (l == headers.end()) return; | ||
| for (auto const& v : l->second.values()) ctx.AddMetadata(name, v); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used for simple types like std::string in range-based loops when it hides the type. Please use std::string const& explicitly.
for (std::string const& v : l->second.values()) ctx.AddMetadata(name, v);References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| auto const expected = | ||
| EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides function return types. Please use std::string explicitly.
std::string const expected =
EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd));References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| auto const expected = | ||
| storage_internal::EncodeFeatureMask(storage_internal::FeatureBit( | ||
| storage_internal::TrackedFeature::kReadHedging)); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides function return types. Please use std::string explicitly.
std::string const expected =
storage_internal::EncodeFeatureMask(storage_internal::FeatureBit(
storage_internal::TrackedFeature::kReadHedging));References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16591 +/- ##
==========================================
+ Coverage 92.44% 92.47% +0.03%
==========================================
Files 2266 2269 +3
Lines 221667 222037 +370
==========================================
+ Hits 204918 205337 +419
+ Misses 16749 16700 -49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cpriti-os
changed the title
kalragauri
left a comment
kalragauri
left a comment
There was a problem hiding this comment.
Changes look good but I see that googleapis/google-cloud-go#20684 is still open, and Go main currently encodes this header with standard base64 (encodeUint32). Should we call out explicitly that the URL-safe alphabet is what the service decodes, and note the Go PR is pending?
Alternately, let's wait until the Go PR is merged so a reader does not assume the two libraries are already aligned.
Also, consider using feat(storage): rather than chore(storage):. This changes what the client sends on the wire for every descriptor open and hedged read, which seems worth a release-note line.
| * is parsed by tooling outside this repository: keep each enumerator on one | ||
| * line, in the form `kName = N,`, and never reuse or renumber a bit. | ||
| */ | ||
| enum class TrackedFeature : std::uint32_t { |
There was a problem hiding this comment.
Bit 1 here is kReadHedging, while in Go bit 1 is featurePCU. Since the comment says this file is parsed by tooling outside the repo, could you name that tooling (or link the registration) so it is clear the service keeps a C++-specific bit table and that these two bits are already registered there?
There was a problem hiding this comment.
yes the service keeps feature bit map per sdk version (so different for C++ and Go)
|
|
||
| // The mask is 32 bits wide, so every enumerator must be a position below 32, | ||
| // and no two features may share a position. | ||
| static_assert(static_cast<std::uint32_t>(TrackedFeature::kMultistreamInMrd) < |
There was a problem hiding this comment.
These asserts grow quadratically as features are added, and tracked_features_test.cc keeps its own copy of the list in kAllFeatures. One constexpr array of all features in this header, checked by a single constexpr function, would cover both and give the test one list to iterate.
cpriti-os
changed the title
Go PR is merged, the encoding/decoding will be the same in Go for standard and URL-safe since the number of bits used are 2, no impact for our readers and dashboard. Go PR is merged to avoid inconsistency in the future when we use more bits. |
kalragauri
left a comment
kalragauri
left a comment
There was a problem hiding this comment.
This PR currently has merge conflicts with main in configure_client_context.h, configure_client_context_test.cc, and grpc/stub.cc following #16600. Please rebase onto main so CreateResumableUpload, QueryResumableUpload, DeleteResumableUpload, and SignBlob keep both #16600's ApplyQueryParameters changes and ApplyRestContextHeaders.
The service cannot currently tell when a client used read hedging or the multi-stream optimization for bidi reads, which makes it impossible to measure the impact of these features from the server side. Other client libraries (e.g. google-cloud-go, `x-goog-storage-go-features`) already report this via a small bitmask header. This change adds `x-goog-storage-cpp-features`, sent as a REST header and as gRPC metadata. The value is a 4-byte big-endian uint32 bitmask encoded as URL-safe base64 (RFC 4648 section 5, `-`/`_` alphabet, with `=` padding). The header is omitted when no tracked feature is in use. Bits (see `storage/internal/tracked_features.h`, the single source of truth): * bit 0, `kMultistreamInMrd`: set on every `BidiReadObject` stream an async `ObjectDescriptor` opens while `storage::EnableMultiStreamOptimizationOption` is true. * bit 1, `kReadHedging`: set on `ReadObject` requests (REST and gRPC) issued by the sync client while read hedging is in effect. The sync path sets the header on the `RestContext`, and `AddIdempotencyToken()` now forwards it (alongside the idempotency token) into the `grpc::ClientContext`, so both transports are covered by a single code path.
Fold the header-copy helper and the idempotency token constant into the file's existing unnamed namespace instead of opening a second one. No behavior change.
Fill the gaps in the test coverage for `x-goog-storage-cpp-features`: * `tracked_features_test`: pin the encodings for masks 3, 0xFC, and a high-byte value (big-endian), place sextets 62 and 63 in every position to prove the URL-safe alphabet, and `static_assert` that the enumerators are distinct bit positions below 32. * `connection_impl_test` (sync, REST `GenericStub`): the header is absent when hedging is enabled but `MaxReadHedgesOption` is 0, when the connection has no hedging thread pools, and on non-`ReadObject` RPCs even with hedging in effect. * `grpc/stub_test` (sync, gRPC): `GrpcStub::ReadObject()` forwards the header from the `RestContext` into the `grpc::ClientContext`, and sends no metadata entry at all when the `RestContext` has none. * `async/connection_impl_open_test`: with the multi-stream optimization on, every `BidiReadObject` stream a descriptor opens carries the header, not just the first one. * `async/connection_impl_read_test`: the plain `ReadObject()` and `ReadObjectRange()` paths never send it.
Rename `AddIdempotencyToken(grpc::ClientContext&, RestContext const&)` to `ApplyRestContextHeaders()`, since it now forwards more than the token. The headers it copies are an explicit allowlist (`kForwardedHeaders`: idempotency token and client feature mask), so REST-only headers on the `RestContext` never leak into gRPC metadata. The string overload of `AddIdempotencyToken()` is unchanged. Also adds `AddFeaturesHeader(grpc::ClientContext&, std::uint32_t)`; it lives here rather than in `tracked_features.h` because `google_cloud_cpp_storage` does not depend on gRPC.
* `FeatureBit()` is now `constexpr` in the header, and the header `static_assert`s that the enumerators are distinct bit positions below 32 (the test-side duplicates are gone). * `AddFeaturesHeader(RestContext&, mask)` and the gRPC overload are no-ops for a zero mask, so callers no longer re-check for the empty encoding. The sync `ReadObject()` and the async open-stream factory use them instead of hand-rolled blocks. * Drop the redundant `& 0xFF` after `static_cast<std::uint8_t>` in `EncodeFeatureMask()`, and the duplicated encoding literal assertion in `connection_impl_test`.
Spell out primitive and function-return types instead of `auto` in the lines this change added, per .gemini/styleguide.md.
* `kAllTrackedFeatures` lists every feature in the header, and one `constexpr` validator (`TrackedFeaturesAreValid()`) behind a single `static_assert` replaces the pairwise asserts: every position is below 32 and the OR of all bits has one bit per feature. The test iterates the same list instead of its own copy. * Document that bit positions are per library (the service keeps a separate bit-to-feature table per client header, populated per release by a tracker that parses this file), and that the service decodes only the URL-safe base64 alphabet.
8ad2950 to
c2629b1
Compare
|
/gcbrun |
Motivation
The service has no way to tell when a client read used read hedging or the multi-stream optimization for bidi reads (
ObjectDescriptor), so the server-side impact of these features cannot be measured. google-cloud-go already reports this withx-goog-storage-go-features(see googleapis/google-cloud-go#20684 for the encoding); this PR adds the C++ equivalent.What
Adds the
x-goog-storage-cpp-featuresheader (REST header / gRPC metadata key), carrying a bitmask of the client-side features in effect for the request.Bits
Single source of truth:
google/cloud/storage/internal/tracked_features.h(machine-parseablekName = N,lines;kAllTrackedFeatureslists them and a singlestatic_assert(TrackedFeaturesAreValid())enforces distinct positions< 32).kMultistreamInMrdObjectDescriptor(AsyncClient::Open/BidiReadObject) runs withstorage::EnableMultiStreamOptimizationOptiontrue. Attached to everyBidiReadObjectstream the descriptor opens (initial, resumes, and additional streams), since all go through the same stream factory.kReadHedgingstorage::ClientissuesReadObjectwhile read hedging is in effect (storage_experimental::EnableReadHedgingOptiontrue,MaxReadHedgesOption > 0, and the hedging thread pools exist). Semantics: the bit means hedging is in effect for the read, not that a hedge was actually dispatched. Attached on both REST and gRPC transports.Encoding contract
uint32bitmask → URL-safe base64 (RFC 4648 §5,-/_alphabet) with=padding.+//) is rejected. fix(storage): use URL-safe base64 for the feature tracking header google-cloud-go#20684 (merged 2026-10-28) switched Go to the same URL-safe encoding, so both libraries now follow the contract the service enforces.tracked_features.h. Positions need not match other SDKs (e.g. Go uses bit 1 for a different feature); bits are never renumbered or reused once released.1 → "AAAAAQ==",2 → "AAAAAg==",0xF8 → "AAAA-A==".Implementation notes
storage/internal/tracked_features.{h,cc}:TrackedFeature(withstatic_asserts that enumerators are distinct bit positions< 32),constexpr FeatureBit(),EncodeFeatureMask()(returns""for mask 0),kFeaturesHeader, andAddFeaturesHeader(rest_internal::RestContext&, mask)(no-op for mask 0). Uses the existinginternal::UrlsafeBase64EncodeWithPadding().AddFeaturesHeader(grpc::ClientContext&, mask)lives instorage/internal/grpc/configure_client_context.{h,cc}becausegoogle_cloud_cpp_storagedoes not depend on gRPC.StorageConnectionImpl::ReadObject()computes the mask from whether hedging is in effect and callsAddFeaturesHeader()on theRestContextnext to the idempotency token.ApplyRestContextHeaders(grpc::ClientContext&, RestContext const&)(renamed from theRestContextoverload ofAddIdempotencyToken) forwards an explicit allowlist (kForwardedHeaders: idempotency token, feature mask) into gRPC metadata, so one code path covers both transports. TheAddIdempotencyToken(ClientContext&, std::string const&)overload is unchanged.MakeOpenStreamFactory()instorage/internal/async/connection_impl.cccallsAddFeaturesHeader()on the per-streamgrpc::ClientContext.Tests
Coverage
internal_tracked_features_testAAAA-A==), 0xFC (AAAA_A==), 0xFFFFFFFF (_____w==); big-endian byte order; mask 0 → empty (header omitted); sextets 62/63 in every position never produce+or/;FeatureBit()per enumerator (alsostatic_asserted, it isconstexpr);AddFeaturesHeader(RestContext&)sets the header and is a no-op for mask 0.GenericStub)internal_connection_impl_testReadObjectcarriesAAAAAg==with hedging in effect; header absent when hedging is off, whenMaxReadHedgesOptionis 0, and when the connection has no hedging thread pools (option set per-call only); never sent onGetObjectMetadata/DeleteObjecteven with hedging in effect. The REST stub forwards allRestContextheaders to curl (CurlRestClient→SetHeaders(context.headers())), whichrest/stub_testalready covers generically.GrpcStub→StorageStubmock)internal_grpc_stub_test,internal_grpc_configure_client_context_testGrpcStub::ReadObject()end-to-end:RestContextheader appears ingrpc::ClientContextmetadata next to the idempotency token; no metadata entry at all when theRestContexthas none.ApplyRestContextHeaders()forwards only the allowlisted headers (a REST-only header is not copied);AddFeaturesHeader(grpc::ClientContext&)sets the header and is a no-op for mask 0.internal_async_connection_impl_open_testBidiReadObjectstream with multi-stream enabled; absent whenEnableMultiStreamOptimizationOptionis false; with multi-stream enabled, every stream the same descriptor opens (first + the background/subsequent streams, incl. retry attempts) carries it.internal_async_connection_impl_read_testReadObject()andReadObjectRange()never send it, even with multi-stream enabled (the default).A mutation check (removing the
AddMetadatacall in the async factory) turns the async tests red.Validation run (latest revision)
bazel test //google/cloud/storage:all(every storage unit test;internal/has no BUILD of its own) — 179/179 PASSED. Earlier full//google/cloud/storage/...run: 209 unit tests PASSED; the only failures werestorage/tests:*_integration_test*/storage/examples:*_samples, which needGOOGLE_CLOUD_PROJECT/ADC and are not runnable here.internal_tracked_features_test,internal_connection_impl_test,internal_grpc_configure_client_context_test,internal_grpc_stub_test,internal_grpc_stub_acl_test,internal_async_connection_impl_open_test,internal_async_connection_impl_read_test,internal_async_connection_impl_test,internal_async_rewriter_connection_impl_test— 9/9 PASSED.ci/cloudbuild/build.sh -t checkers-pr(docker) — PASSED (exit 0, no diff).GOOGLE_CLOUD_CPP_FAST_CHECKERS=1 ci/cloudbuild/builds/checkers.sh— SKIPPED locally (typosnot installed); covered by the docker run.ci/cloudbuild/build.sh -t clang-tidy-pr— SKIPPED (thefedora-latest-cmakeimage builds every dependency from source). Instead clang-tidy 22.1.8 with the repo's.clang-tidyand Bazel's compile flags was run on every.ccthis PR touches: no findings on any PR-touched line or file (the newer clang-tidy flags pre-existing sites in untouched headers via checks CI's pinned version does not have)..cmakesource lists were updated alongside the.bzllists).