[OTel C++] Add experimental optional locality label available to client per-attempt metrics #36254

yashykt · 2024-04-04T02:17:52Z

As per grpc/proposal#419, the experimental optional label grpc.lb.locality is added to the follow per-call metrics -

grpc.client.attempt.duration
grpc.client.attempt.sent_total_compressed_message_size
grpc.client.attempt.rcvd_total_compressed_message_size

…pt metrics

markdroth

This looks like the right approach! There are a few noteworthy comments, though,

PLMK if you have any questions. Thanks!

src/core/ext/xds/xds_client_stats.h

src/core/ext/xds/xds_cluster.h

src/core/lib/channel/call_tracer.h

src/cpp/ext/otel/otel_client_call_tracer.h

src/cpp/ext/otel/otel_plugin.h

src/cpp/ext/otel/key_value_iterable.h

test/cpp/ext/otel/otel_plugin_test.cc

markdroth

Only one really significant comment left, which is the one in xds_cluster_stats.h.

PLMK if you have any questions. Thanks!

src/core/lib/channel/call_tracer.h

src/core/ext/xds/xds_client_stats.h

src/core/resolver/xds/xds_dependency_manager.cc

src/core/ext/xds/xds_cluster.h

src/cpp/ext/otel/key_value_iterable.h

src/cpp/ext/otel/otel_plugin.h

test/core/xds/xds_cluster_resource_type_test.cc

src/core/ext/xds/xds_client_stats.h

src/core/load_balancing/xds/cds.cc

src/core/ext/xds/xds_cluster.h

yijiem

Thanks Yash!

yijiem · 2024-04-06T00:07:29Z

src/core/load_balancing/xds/xds_wrr_locality.cc

@@ -167,15 +167,15 @@ absl::Status XdsWrrLocalityLb::UpdateLocked(UpdateArgs args) {
  }
  auto config = args.config.TakeAsSubclass<XdsWrrLocalityLbConfig>();
  // Scan the addresses to find the weight for each locality.
-  std::map<std::string, uint32_t> locality_weights;
+  std::map<RefCountedStringValue, uint32_t> locality_weights;


Does this needs to be std::map<RefCountedStringValue, uint32_t, RefCountedStringValueLessThan>?

Haven't gotten a complaint from the compiler, maybe because I'm just comparing it with other RefCountedStringValue. We can add it if needed.

We define operator<(RefCountedStringValue, RefCountredStringValue), so it shouldn't be necessary to use RefCountedStringValueLessThan unless we need heterogeneous lookups.

yijiem · 2024-04-06T00:22:05Z

src/cpp/ext/otel/otel_plugin.h

@@ -366,6 +365,17 @@ class OpenTelemetryPlugin : public grpc_core::StatsPlugin {
  };

  // StatsPlugin:


Maybe move this comment to line 379 which marks the start of the StatsPlugin's methods.

I see, I didn't realize what you meant by StatsPlugin's methods

yijiem · 2024-04-06T00:29:23Z

src/cpp/ext/otel/otel_plugin.cc

+  switch (key) {
+    case grpc_core::ClientCallTracer::CallAttemptTracer::OptionalLabelKey::
+        kLocality:
+      return kLocality;
+    default:
+      grpc_core::Crash("Illegal OptionalLabelKey index");
+  }


Since we only have one valid OptionalLabelKey that the user can set for now, we could also write it like:

GPR_ASSERT(key == grpc_core::ClientCallTracer::CallAttemptTracer::OptionalLabelKey::kLocality); return kLocality;

The current structure makes it easier to add new labels, and I think the compiler would be smart enough to optimize it.

markdroth

Just one small remaining issue. Feel free to merge after addressing.

src/core/load_balancing/xds/xds_cluster_impl.cc

src/core/ext/xds/xds_cluster.h

[OTel C++] Make optional locality label available to client per-attem…

3f75a77

…pt metrics

yashykt requested a review from markdroth as a code owner April 4, 2024 02:17

github-actions bot added lang/c++ lang/core labels Apr 4, 2024

yashykt changed the title ~~[OTel C++] Make optional locality label available to client per-attempt metrics~~ [OTel C++] Add experimental optional locality label available to client per-attempt metrics Apr 4, 2024

yashykt added the release notes: yes Indicates if PR needs to be in release notes label Apr 4, 2024

yashykt requested a review from yijiem April 4, 2024 02:19

yashykt added 2 commits April 4, 2024 02:24

Cleanup

e69c58e

Try std::vector<std::pair<>> at some places and fix Python

2d1f6e7

github-actions bot added the lang/Python label Apr 4, 2024

grpc-checks bot added per-call-memory/increase per-channel-memory/neutral bloat/none per-call-memory/decrease bloat/improvement and removed per-call-memory/increase bloat/none per-call-memory/decrease labels Apr 4, 2024

markdroth reviewed Apr 4, 2024

View reviewed changes

yashykt added 4 commits April 4, 2024 20:35

Reviewer comments

1bc9700

Reviewer comments

3ff4d2b

Cleanup

0355019

Cleanup

4fdc52a

grpc-checks bot added per-call-memory/neutral per-channel-memory/increase per-call-memory/decrease and removed per-call-memory/increase per-channel-memory/neutral per-call-memory/neutral labels Apr 4, 2024

grpc-checks bot added per-call-memory/decrease and removed per-call-memory/increase labels Apr 4, 2024

markdroth reviewed Apr 5, 2024

View reviewed changes

Reviewer comments

1a28b31

markdroth reviewed Apr 5, 2024

View reviewed changes

src/core/ext/xds/xds_client_stats.h Outdated Show resolved Hide resolved

src/core/load_balancing/xds/cds.cc Outdated Show resolved Hide resolved

src/core/ext/xds/xds_cluster.h Outdated Show resolved Hide resolved

yashykt added 2 commits April 5, 2024 21:35

Reviewer comments

e62561e

Merge branch 'master' into OTelOptionalLabelsOnPerCall

d02b765

grpc-checks bot added per-call-memory/increase per-channel-memory/neutral and removed per-call-memory/decrease per-channel-memory/increase labels Apr 5, 2024

Fix test

7807486

grpc-checks bot added per-call-memory/decrease and removed per-call-memory/increase labels Apr 5, 2024

yashykt added 4 commits April 6, 2024 00:03

Fix sanity

7fae5f0

Revert third_party/protobuf changes

f1e40af

Fix sanity

a99b904

Revert tools changes

94eb2bf

grpc-checks bot added per-call-memory/neutral and removed per-call-memory/decrease labels Apr 6, 2024

yijiem approved these changes Apr 6, 2024

View reviewed changes

markdroth approved these changes Apr 6, 2024

View reviewed changes

src/core/load_balancing/xds/xds_cluster_impl.cc Outdated Show resolved Hide resolved

Reviewer comments

5207709

markdroth reviewed Apr 8, 2024

View reviewed changes

src/core/ext/xds/xds_cluster.h Outdated Show resolved Hide resolved

Fixes

c5390c9

markdroth approved these changes Apr 8, 2024

View reviewed changes

grpc-checks bot added per-call-memory/increase and removed per-call-memory/neutral labels Apr 8, 2024

copybara-service bot closed this in 70839a9 Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OTel C++] Add experimental optional locality label available to client per-attempt metrics #36254

[OTel C++] Add experimental optional locality label available to client per-attempt metrics #36254

yashykt commented Apr 4, 2024

markdroth left a comment

markdroth left a comment

yijiem left a comment

yijiem Apr 6, 2024

yashykt Apr 6, 2024

markdroth Apr 8, 2024

yijiem Apr 6, 2024

yashykt Apr 6, 2024

yijiem Apr 6, 2024

yashykt Apr 6, 2024

markdroth left a comment

		@@ -366,6 +365,17 @@ class OpenTelemetryPlugin : public grpc_core::StatsPlugin {
		};

		// StatsPlugin:

[OTel C++] Add experimental optional locality label available to client per-attempt metrics #36254

[OTel C++] Add experimental optional locality label available to client per-attempt metrics #36254

Conversation

yashykt commented Apr 4, 2024

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

yijiem left a comment

Choose a reason for hiding this comment

yijiem Apr 6, 2024

Choose a reason for hiding this comment

yashykt Apr 6, 2024

Choose a reason for hiding this comment

markdroth Apr 8, 2024

Choose a reason for hiding this comment

yijiem Apr 6, 2024

Choose a reason for hiding this comment

yashykt Apr 6, 2024

Choose a reason for hiding this comment

yijiem Apr 6, 2024

Choose a reason for hiding this comment

yashykt Apr 6, 2024

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment