Repository navigation
Conversation
product-auto-label
Bot
added
the
api: bigtable
![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 refactors the ConnectivityErrorCount metric class to emit metrics per attempt in PostCall instead of accumulating them until OnDone, and ensures that cluster and zone resource labels are cleared across retries when response parameters are missing. It also introduces a helper function IsConnectivityErrorImpl to centralize connectivity error detection. The reviewer feedback suggests factoring out the duplicated logic for updating and clearing resource labels into a shared helper function UpdateResourceLabels in the anonymous namespace, which is repeated across four different PostCall implementations.
5f1aa01 to
a5f3a30
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors metric collection in table_schema_metrics.cc by introducing helper functions UpdateResourceLabels and IsConnectivityErrorImpl to reduce duplication and simplify logic across latency and connectivity error tracking. It also removes the stateful error counting in ConnectivityErrorCount in favor of recording errors per attempt, and adds comprehensive unit tests to verify cluster/zone resetting behavior and the "three-signal rule" for connectivity errors. The reviewer feedback focuses on optimizing performance by avoiding unnecessary copies of ResponseParams protobuf messages (by passing them by value/moving them) and reducing nesting by returning early.
Record connectivity_error_count per attempt in PostCall using all three server-reached signals (ResponseParams, server-timing, and bigtable-peer-info) on non-OK, non-DEADLINE_EXCEEDED RPCs, and clear per-attempt cluster/zone resource labels when ResponseParams is absent on retries.
a5f3a30 to
cf38749
Compare
scotthart
added
the
do not merge
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the table schema metrics implementation by extracting common logic into helper functions, such as UpdateResourceLabels and IsConnectivityErrorImpl, and simplifies metric tracking for connectivity errors. It also adds comprehensive unit tests to verify cluster/zone stickiness, resetting behavior, and connectivity error detection rules. The reviewer suggested optimizing UpdateResourceLabels by passing response_params as an rvalue reference (&&) to avoid unnecessary move-construction of the std::optional wrapper.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16656 +/- ##
==========================================
- Coverage 92.53% 92.50% -0.03%
==========================================
Files 2269 2269
Lines 222896 223302 +406
==========================================
+ Hits 206250 206567 +317
- Misses 16646 16735 +89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Fixes three issues in
ConnectivityErrorCountand per-attemptTableSchemaMetriclabel handling:ConnectivityErrorCount::PostCallpreviously incrementednum_errors_wheneverresource_labels_.cluster.empty() || resource_labels_.zone.empty() || IsConnectivityError(...), whereIsConnectivityErroronly checkedstatus.code() != kDeadlineExceeded && !HasServerTiming(client_context). Consequently, successful (OK) RPCs withoutResponseParamsorserver-timing(e.g., DirectPath RPCs carryingbigtable-peer-info, emulator calls, or stripped headers) recorded a connectivity error (1), and non-OKRPCs that reached Bigtable (evidenced byResponseParamsorbigtable-peer-infowithoutserver-timing) were also misclassified as connectivity errors.ConnectivityErrorCountaccumulatednum_errors_across retries and emitted a single counter value inOnDoneunder the final attempt's status/cluster/zone labels instead of recording1or0per attempt inPostCallwith each attempt's ownstatus,cluster, andzone.AttemptLatency,AttemptLatency2,ServerLatency, andConnectivityErrorCount) did not clearresource_labels_.clusterandresource_labels_.zonewhenResponseParamswas absent on a subsequent attempt, leaking the previous attempt's cluster/zone onto retries that failed before reaching a cluster.Changes
IsConnectivityError(andIsConnectivityErrorImpl) to require!status.ok() && status.code() != StatusCode::kDeadlineExceededand the absence of all three server-reached metadata signals (ResponseParams,server-timing, and validbigtable-peer-info).connectivity_error_count(1or0) per attempt inConnectivityErrorCount::PostCallrather than aggregating inOnDone.resource_labels_.clusterandresource_labels_.zoneinAttemptLatency::PostCall,AttemptLatency2::PostCall,ServerLatency::PostCall, andConnectivityErrorCount::PostCallwhenResponseParamsis absent (while preserving sticky cluster/zone labels on operation-levelOperationLatency).google/cloud/bigtable/internal/table_schema_metrics_test.cc.