Skip to content

feat(bigtable): add AttemptLatency2 metric and populate peer info labels#16095

Merged
elinorli merged 2 commits into
googleapis:mainfrom
elinorli:directpath
May 19, 2026
Merged

feat(bigtable): add AttemptLatency2 metric and populate peer info labels#16095
elinorli merged 2 commits into
googleapis:mainfrom
elinorli:directpath

Conversation

@elinorli
Copy link
Copy Markdown
Contributor

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.

@elinorli elinorli requested a review from a team as a code owner May 13, 2026 23:55
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label May 13, 2026
Copy link
Copy Markdown

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

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 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();
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

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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed. Let's avoid making a copy if we don't have to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread google/cloud/bigtable/internal/metrics.cc Outdated
auto iter = metadata.find("bigtable-peer-info");
if (iter == metadata.end()) return absl::nullopt;
std::string decoded;
if (!absl::Base64Unescape(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

check if we need [WebSafeBase64Unescape]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread google/cloud/bigtable/internal/metrics.cc Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 99.02913% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.71%. Comparing base (cf931ff) to head (c621978).

Files with missing lines Patch % Lines
...oud/bigtable/internal/operation_context_factory.cc 77.77% 2 Missing ⚠️
google/cloud/bigtable/internal/metrics.cc 98.21% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16095      +/-   ##
==========================================
+ Coverage   92.70%   92.71%   +0.01%     
==========================================
  Files        2353     2353              
  Lines      218354   218643     +289     
==========================================
+ Hits       202419   202719     +300     
+ Misses      15935    15924      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

absl::optional<google::bigtable::v2::ResponseParams>
GetResponseParamsFromTrailingMetadata(
grpc::ClientContext const& client_context);
absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use std::optional instead of absl::optional in new code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


absl::optional<google::bigtable::v2::PeerInfo> GetPeerInfoFromTrailingMetadata(
grpc::ClientContext const& client_context) {
auto metadata = client_context.GetServerTrailingMetadata();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::nullopt instead of absl::nullopt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

}
google::bigtable::v2::PeerInfo p;
if (p.ParseFromString(decoded)) return p;
return absl::nullopt;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::nullopt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


void AttemptLatency2::PreCall(opentelemetry::context::Context const&,
PreCallParams const& p) {
attempt_start_ = std::move(p.attempt_start);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

p is passed by const& and cannot be moved from.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. I've also changed it for AttemptLatency.

SetServerMetadata(client_context, server_metadata);

EXPECT_THAT(GetPeerInfoFromTrailingMetadata(client_context),
Eq(absl::nullopt));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::nullopt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

SetServerMetadata(client_context, server_metadata);

EXPECT_THAT(GetPeerInfoFromTrailingMetadata(client_context),
Eq(absl::nullopt));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::nullopt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread google/cloud/bigtable/internal/metrics_test.cc
@sushanb
Copy link
Copy Markdown

sushanb commented May 15, 2026

Looks good. You can run this client with quickstart and verify that attempt_latencies2 is being populated from your workload.

elinorli added 2 commits May 18, 2026 15:43
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`.
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`.
@elinorli elinorli merged commit 1735f56 into googleapis:main May 19, 2026
53 of 59 checks passed
@elinorli elinorli deleted the directpath branch May 19, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants