feat(bigtable): add AttemptLatency2 metric and populate peer info labels#16095
feat(bigtable): add AttemptLatency2 metric and populate peer info labels#16095elinorli wants to merge 1 commit into
Conversation
This introduces the `AttemptLatency2` metric for DirectPath to record attempt latencies with the fields extracted from the decoded `PeerInfo` trailing metadata, populating `peer_info_labels_` and forwarding them to `IntoLabelMap`. Also added `AttemptLatency2Test` to test the newly populated peer info labels. Refactored `SetClusterZone` to use the new helper function `CreateServerMetadata`.
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 introduces the AttemptLatency2 metric, which extends latency tracking by including peer information such as transport type, region, and subzone extracted from gRPC trailing metadata. The implementation includes new data structures for peer labels, utility functions for decoding metadata, and the integration of this metric into various Bigtable operations. Review feedback suggests a performance optimization in GetPeerInfoFromTrailingMetadata to use a constant reference when accessing trailing metadata to avoid an unnecessary copy of the multimap.
|
|
||
| absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata( | ||
| grpc::ClientContext const& client_context) { | ||
| auto metadata = client_context.GetServerTrailingMetadata(); |
There was a problem hiding this comment.
The GetServerTrailingMetadata() method returns a reference to a std::multimap. Using auto here results in a copy of the entire map, which is inefficient. Prefer using auto const& to avoid the unnecessary copy.
auto const& metadata = client_context.GetServerTrailingMetadata();
There was a problem hiding this comment.
Agreed. Let's avoid making a copy if we don't have to.
|
|
||
| absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata( | ||
| grpc::ClientContext const& client_context) { | ||
| auto metadata = client_context.GetServerTrailingMetadata(); |
There was a problem hiding this comment.
bigtable-peer-info is sent as initialmetadata. so i would check it it initial metadata and if it is not present, check it in GetServerTrailingMetadata
| auto iter = metadata.find("bigtable-peer-info"); | ||
| if (iter == metadata.end()) return absl::nullopt; | ||
| std::string decoded; | ||
| if (!absl::Base64Unescape( |
There was a problem hiding this comment.
check if we need [WebSafeBase64Unescape]
| } | ||
|
|
||
| std::string TransportTypeToString( | ||
| google::bigtable::v2::PeerInfo::TransportType type) { |
There was a problem hiding this comment.
I think we can use implict proto conversion here. absl::AsciiStrToLower(google::bigtable::v2::PeerInfo::TransportType_Name(type));
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16095 +/- ##
==========================================
- Coverage 92.70% 92.70% -0.01%
==========================================
Files 2353 2353
Lines 218354 218612 +258
==========================================
+ Hits 202428 202664 +236
- Misses 15926 15948 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| absl::optional<google::bigtable::v2::ResponseParams> | ||
| GetResponseParamsFromTrailingMetadata( | ||
| grpc::ClientContext const& client_context); | ||
| absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata( |
There was a problem hiding this comment.
Let's use std::optional instead of absl::optional in new code.
|
|
||
| absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata( | ||
| grpc::ClientContext const& client_context) { | ||
| auto metadata = client_context.GetServerTrailingMetadata(); |
There was a problem hiding this comment.
Agreed. Let's avoid making a copy if we don't have to.
| auto metadata = client_context.GetServerTrailingMetadata(); | ||
| // Base64 encoded peer info header key defined by the server. | ||
| auto iter = metadata.find("bigtable-peer-info"); | ||
| if (iter == metadata.end()) return absl::nullopt; |
There was a problem hiding this comment.
std::nullopt instead of absl::nullopt
| } | ||
| google::bigtable::v2::PeerInfo p; | ||
| if (p.ParseFromString(decoded)) return p; | ||
| return absl::nullopt; |
|
|
||
| void AttemptLatency2::PreCall(opentelemetry::context::Context const&, | ||
| PreCallParams const& p) { | ||
| attempt_start_ = std::move(p.attempt_start); |
There was a problem hiding this comment.
p is passed by const& and cannot be moved from.
| SetServerMetadata(client_context, server_metadata); | ||
|
|
||
| EXPECT_THAT(GetPeerInfoFromTrailingMetadata(client_context), | ||
| Eq(absl::nullopt)); |
| SetServerMetadata(client_context, server_metadata); | ||
|
|
||
| EXPECT_THAT(GetPeerInfoFromTrailingMetadata(client_context), | ||
| Eq(absl::nullopt)); |
| AttemptLatency2 attempt_latency("my-instrument-scope", mock_provider); | ||
| ResourceLabels resource_labels{"my-project-id", "my-instance", "my-table", "", | ||
| ""}; | ||
| DataLabels data_labels{"my-method", "my-streaming", "my-client-name", |
There was a problem hiding this comment.
This formatting is unusual. Please remove the extra white space between arguments.
3 participants
This introduces the
AttemptLatency2metric for DirectPath to record attempt latencies with the fields extracted from the decodedPeerInfotrailing metadata, populatingpeer_info_labels_and forwarding them toIntoLabelMap.Also added
AttemptLatency2Testto test the newly populated peer info labels. RefactoredSetClusterZoneto use the new helper functionCreateServerMetadata.