[Metrics] gRPC Non-Per-Call Metrics framework implementation #35871

yijiem · 2024-02-09T00:44:34Z

No description provided.

yashykt · 2024-02-12T22:47:09Z

src/core/lib/channel/metrics.h

+  // Creates instrument in the GlobalInstrumentsRegistry.
+  static GlobalUInt64CounterHandle RegisterUInt64Counter(
+      absl::string_view name, absl::string_view description,
+      absl::string_view unit, std::vector<absl::string_view> label_keys,


Note that we want the MakeValueSet optimization here, but I see the TODO down below, so I'm fine with this as a first iteration

src/core/lib/channel/metrics.h

yashykt · 2024-02-12T22:56:34Z

test/core/channel/metrics_test.cc

+}
+
+template <class T>
+std::string MakeKey(T t) {


I think this is weird. I would much prefer you just create a FakeCounter and a FakeHistogram. That's going to make much more sense.

I removed this. The FakeStatsPlugin creates an instrument for each instruments in the GlobalInstrumentsRegistry. It now uses the index to find the instrument that it created (as it should be used IIUC).

yashykt · 2024-02-12T23:02:38Z

src/core/lib/channel/metrics.h

+      absl::string_view unit, std::vector<absl::string_view> label_keys,
+      std::vector<absl::string_view> optional_label_keys);
+
+  static const GlobalCounterDescriptor& GetCounterDescriptor(


I don't see the point of the GetCounterDescriptor() or GetHistogramDescriptor() methods. We could just use the counters() or histograms() methods and use the handle as an index directly?

If you are thinking of this as just a convenience method, I'm fine with it

Removed. I combined counters() and histograms() into a single instruments() method, PTAL.

yashykt · 2024-02-12T23:13:14Z

src/core/lib/channel/metrics.h

+namespace grpc_core {
+
+// Static only.
+class GlobalInstrumentsRegistry {


"Static initialization"

yashykt · 2024-02-12T23:14:46Z

src/core/lib/channel/metrics.h

+    std::vector<absl::string_view> label_keys;
+    std::vector<absl::string_view> optional_label_keys;
+  };
+  struct GlobalHandle {


I think we should make it clear that we are using the handle as an index in docs, and that that is an API. (unless, we want to hide the fact that handle is just an index)

Added some comment about the index. I think we can expose it so that StatsPlugins can use it to uniquely identify an instrument later with the handle. PTAL.

GlobalInstrumentHandle? I think grpc_core::GlobalHandle sounds very generic

markdroth

This looks like a great start!

Please let me know if you have any questions. Thanks!

src/core/lib/channel/metrics.h

test/core/channel/metrics_test.cc

yijiem

Thanks Mark!

src/core/lib/channel/metrics.cc

yijiem · 2024-02-14T01:29:30Z

src/core/lib/channel/metrics.h

+namespace grpc_core {
+
+// Static only.
+class GlobalInstrumentsRegistry {


src/core/lib/channel/metrics.h

test/core/channel/metrics_test.cc

markdroth

This looks great! Remaining comments are minor.

Please let me know if you have any questions. Thanks!

src/core/lib/channel/metrics.cc

test/core/channel/metrics_test.cc

src/core/lib/channel/metrics.h

markdroth · 2024-02-20T22:32:54Z

This looks great! The only remaining issue is the one about not wrapping the ChannelScope object.

markdroth

Looks great, modulo one minor comment. Feel free to merge after addressing.

Nice work!

src/core/lib/channel/metrics.h

I realized we need one more thing.

src/core/lib/channel/metrics.cc

markdroth

This looks fabulous! Feel free to merge after addressing the remaining comment.

markdroth · 2024-02-22T22:39:31Z

test/core/channel/metrics_test.cc

          channel_filter)
      : channel_filter_(std::move(channel_filter)) {
    GlobalInstrumentsRegistry::ForEach(
        [this](const GlobalInstrumentsRegistry::GlobalInstrumentDescriptor&
                   descriptor) {
+          if (!descriptor.enable_by_default) {


Please also add a test that covers adding a metric that is not enabled by default.

markdroth

Looks great!

yijiem added 3 commits February 5, 2024 13:57

initial metrics framework implementation

ddab469

update tests

d359347

add lock

fcb2a78

yijiem added the release notes: no Indicates if PR should not be in release notes label Feb 9, 2024

yijiem requested a review from yashykt February 9, 2024 00:44

github-actions bot added the lang/core label Feb 9, 2024

yijiem added 2 commits February 8, 2024 16:47

sanitize

9c7a916

cleanup

66611f0

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

yashykt marked this pull request as ready for review February 12, 2024 22:45

yashykt requested review from ctiller and markdroth February 12, 2024 22:45

yashykt reviewed Feb 12, 2024

View reviewed changes

yijiem added 2 commits February 12, 2024 17:25

new idea

8170476

clarify comments about the index

feb3636

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

markdroth reviewed Feb 13, 2024

View reviewed changes

yijiem added 2 commits February 14, 2024 14:31

partial review

d2dabb2

review

8be3fb1

yijiem commented Feb 16, 2024

View reviewed changes

grpc-checks bot removed the per-call-memory/increase label Feb 16, 2024

markdroth reviewed Feb 17, 2024

View reviewed changes

src/core/lib/channel/metrics.cc Outdated Show resolved Hide resolved

test/core/channel/metrics_test.cc Outdated Show resolved Hide resolved

test/core/channel/metrics_test.cc Outdated Show resolved Hide resolved

src/core/lib/channel/metrics.h Outdated Show resolved Hide resolved

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

yijiem added 3 commits February 20, 2024 13:12

review

437d12e

compile

139c06c

compile

ff76e8b

yijiem added 2 commits February 21, 2024 13:40

compile

d73e048

review

102f976

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

markdroth previously approved these changes Feb 22, 2024

View reviewed changes

src/core/lib/channel/metrics.h Outdated Show resolved Hide resolved

markdroth changed the title ~~[Metrics] gRPC Metrics framework implementation~~ [Metrics] gRPC Non-Per-Call Metrics framework implementation Feb 22, 2024

markdroth reviewed Feb 22, 2024

View reviewed changes

src/core/lib/channel/metrics.h Show resolved Hide resolved

markdroth reviewed Feb 22, 2024

View reviewed changes

src/core/lib/channel/metrics.cc Outdated Show resolved Hide resolved

review

1782ec5

markdroth approved these changes Feb 22, 2024

View reviewed changes

review

37d087a

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

sanitize

86ebe48

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

markdroth mentioned this pull request Feb 22, 2024

[WRR] implement non-per-call metrics #35977

Closed

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

markdroth approved these changes Feb 23, 2024

View reviewed changes

copybara-service bot closed this in fb4c043 Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics] gRPC Non-Per-Call Metrics framework implementation #35871

[Metrics] gRPC Non-Per-Call Metrics framework implementation #35871

yijiem commented Feb 9, 2024

yashykt Feb 12, 2024

yashykt Feb 12, 2024

yijiem Feb 13, 2024

yashykt Feb 12, 2024

yashykt Feb 12, 2024

yijiem Feb 13, 2024

yashykt Feb 12, 2024

yijiem Feb 14, 2024

yashykt Feb 12, 2024

yijiem Feb 13, 2024

yashykt Feb 21, 2024

markdroth left a comment

yijiem left a comment

yijiem Feb 14, 2024

markdroth left a comment

markdroth commented Feb 20, 2024

markdroth left a comment

markdroth left a comment

markdroth Feb 22, 2024

yijiem Feb 22, 2024

markdroth left a comment

[Metrics] gRPC Non-Per-Call Metrics framework implementation #35871

[Metrics] gRPC Non-Per-Call Metrics framework implementation #35871

Conversation

yijiem commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth commented Feb 20, 2024

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment