experimental/stats: Add metrics registry #7349

zasweq · 2024-06-25T00:46:17Z

This PR adds the instrument registry for Non Per Call Metrics. It also moves Metrics symbol to external, and defined a MetricRecorder type for non per call metrics.

The next PR will be the OpenTelemetry component usage of this global registry and implementation of MetricRecorder, including the merging of default metrics by shifting default metrics to a function as per Yashes suggestion.

RELEASE NOTES: N/A

codecov · 2024-06-25T00:50:09Z

Codecov Report

Attention: Patch coverage is 67.03297% with 30 lines in your changes missing coverage. Please review.

Project coverage is 81.54%. Comparing base (4dd7f55) to head (1ec4e54).
Report is 36 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7349      +/-   ##
==========================================
+ Coverage   80.58%   81.54%   +0.95%     
==========================================
  Files         349      350       +1     
  Lines       34056    26843    -7213     
==========================================
- Hits        27445    21889    -5556     
+ Misses       5431     3770    -1661     
- Partials     1180     1184       +4

Files	Coverage Δ
experimental/stats/metricregistry.go	`93.44% <93.44%> (ø)`
experimental/stats/metrics.go	`13.33% <13.33%> (ø)`

... and 302 files with indirect coverage changes

zasweq · 2024-06-26T00:48:15Z

Got to offline discussion items, and moved symbols around to where we discussed.

dfawley · 2024-07-01T20:45:21Z

stats/stats.go

+// Metric is an identifier for a metric.
+type Metric string


Shall we create a metrics.go for these new things?

Ah, good point. Done.

dfawley · 2024-07-01T20:47:19Z

stats/stats.go

+	// Metrics are the set of Metrics to initialize.
+	Metrics map[Metric]bool


Shouldn't this be private? Maybe a ToSlice method instead?

I export this to read this field in OTel construction (determines whether a metric is created or not)...do you want me to expose a getter for a map and copy? That would prevent the user from doing operations on this?

dfawley · 2024-07-01T22:11:26Z

experimental/stats/instrumentregistry/instrument_registry.go

+
+import "google.golang.org/grpc/stats"
+
+// Int64CountHandle is a typed handle for a int count instrument. This handle is


*an int - throughout.

dfawley · 2024-07-01T22:11:48Z

experimental/stats/instrumentregistry/instrument_registry.go

+// Int64CountHandle is a typed handle for a int count instrument. This handle is
+// passed at the recording point in order to know which instrument to record on.
+type Int64CountHandle struct {
+	Index int


Does this need to be exported?

Yes, both the OpenTelemetry component and the MetricsRecorderList (the next two PRs) read this index out. (In MetricsRecorderList, it uses the index to verify the labels/optionalLabels (which will log at error and eat call, not panic like Java as discussed offline). OpenTelemetry uses this index as the invariant of instrument registry that it creates metrics from and needs the Index to know which created metric to record. (In the code I have, I create noops for unregistered metrics as well, and the index will match to that and be eaten).

dfawley · 2024-07-01T22:13:13Z

experimental/stats/instrumentregistry/instrument_registry.go

+// DefaultMetrics are the default metrics registered through global instruments
+// registry. This is written to at initialization time only, and is read
+// only after initialization.
+var DefaultMetrics = make(map[stats.Metric]bool)


I don't think we want users to have direct access to this map? I think they just need a Register type function exposed.

We discussed this offline; I thought one of the requirements you gave me was to explicitly expose this, so users that write their own stats plugins/non per call interface can have access to it.

They need to read the default metrics but not directly access a set of them.

Also: why not use the Metrics set type for this?

Oh ok good point. I did add Merge() so yeah this should be a MetricsSet, is having the exported type be a MetricsSet good enough for your point of reading but not accessing it: "They need to read the default metrics but not directly access a set of them."

This merge will be called at runtime in OpenTelemetry for the default metrics as discussed offline.

dfawley · 2024-07-01T22:14:32Z

internal/stats/instrumentregistry/instrument_registry.go

+ *
+ */
+
+// Package instrumentregistry is a instrument registry that gRPC components can


dfawley · 2024-07-01T22:14:52Z

internal/stats/instrumentregistry/instrument_registry.go

+	"google.golang.org/grpc/stats"
+)
+
+// InstrumentDescriptor is a data of a registered instrument (metric).


"the data for"?

dfawley · 2024-07-01T22:15:43Z

internal/stats/instrumentregistry/instrument_registry.go

+	Name string
+	// Description is the description of this metric.
+	Description string
+	// Unit is the unit of this metric.


Can we delete these comments? Or make them less tautological?

Yeah these are just for vet, is there a way to ignore vet for exported fields like this that are self explanatory?

You can always improve them to make them more useful. Like give examples for units, explain what labels are used for, etc.

dfawley · 2024-07-01T22:17:01Z

internal/stats/instrumentregistry/instrument_registry.go

+)
+
+// InstrumentDescriptor is a data of a registered instrument (metric).
+type InstrumentDescriptor struct {


Aren't these things that third-party plugins will eventually need to work with? (I.e. should be in experimental?)

Oh hmmmm you're right. So the only things kept internal are what a gRPC component calls into, and this will need to be read by external users in their stats plugins so they can create instruments based off it.

I asked about this in the chat; thinking about it do I need to move the whole instrument registry into experimental actually? A user can also write their own balancer/resolver, which should be able to register instruments to eventually record on so I think that all the symbols/logic need to be external (in experimental). What do you think?

Discussed in chat with Yash; decided to move this whole instrument registry to experimental and make externally visible.

dfawley · 2024-07-01T22:18:47Z

internal/stats/instrumentregistry/instrument_registry.go

+// NOTE: this function must only be called during initialization time (i.e. in
+// an init() function), and is not thread-safe. If multiple instruments are
+// registered with the same name, this function will panic.
+func RegisterInt64Count(name string, desc string, unit string, labels []string, optionalLabels []string, def bool) instrumentregistry.Int64CountHandle {


How about having the user pass the InstrumentDescriptor directly?

Hmm ok. Thinking about it that would allow scaling up fields if needed in a backward compatible way if we decide we want to persist another piece of information here. Underlying though I'll persist around a copy of the struct not the same pointer as a user.

You can pass by value instead of pointer to avoid the need to manually copy.

I was thinking about it too now that I use a metrics set for defaults and there's a metrics type I might just have the inst desc take a Metric, and have the user pass in a Metric for explicit types and no need to typecast in this registry.

dfawley

If we check this in without any code using it, then we should probably be prepared to make a lot of changes here.

dfawley · 2024-07-10T20:11:43Z

experimental/stats/metricregistry.go

+package stats
+
+import (
+	"log"


We don't use "log"

Ah whoops I had this for panic formatting. Switched to logger.Fatalf().

dfawley · 2024-07-10T20:14:01Z

experimental/stats/metricregistry.go

+	Name stats.Metric
+	// Description is the description of this metric.
+	Description string
+	// Unit is the unit of this metric (e.g. entries, milliseconds).


Isn't OTel opposed to using "milliseconds" and instead prefer to use "seconds" for most/all things time related?

Oh yeah I thought the A66 metrics were /milliseconds, but they are defined as /s (https://github.com/grpc/proposal/blob/master/A66-otel-stats.md#client-per-attempt-instruments) which is why we had those bucket counts.

dfawley · 2024-07-10T20:15:40Z

experimental/stats/metricregistry.go

+
+// MetricDescriptor is the data for a registered metric.
+type MetricDescriptor struct {
+	// Name is the name of this metric. This name must be unique across whole


Docstrings on fields don't follow this convention like types/functions. You can just say "The name of the metric". Throughout. Maybe that's why it seems so redundant to me "Name is the name". Just say "The name".

https://google.github.io/styleguide/go/decisions#doc-comments

Maybe I'll just delete these comments.

Actually switched comments for field to // The Field...

dfawley · 2024-07-10T20:19:47Z

experimental/stats/metricregistry.go

+// is passed at the recording point in order to know which metric to record
+// on.
+type Int64CountHandle struct {
+	MetricDescriptor *MetricDescriptor


Unexport this field throughout? Or maybe type Int64CountHandle MetricDescriptor? Or type Int64CountHandle *MetricDescriptor?

Hmm ok so we could do an alias too? I needed this exported for OpenTelemetry to read the pointer to look into it's data structures it builds out for metrics. (I guess the metrics recorder list could live here for the assertion on labels/optional labels length and access unexported fields).

If I make it a pointer alias; I can't define functions on it. I tried making it a alias to metricDescriptor but the problem is if I typecast the descriptor into a xxxHandle it changes address. How do we keep it pointers so it can link alongside calling methods on handle returned?

Maybe you can point to a playground link illustrating the things you tried? The code as-is is still a struct so I can't see what you did.

dfawley · 2024-07-10T20:20:57Z

experimental/stats/metricregistry.go

+}
+
+// Record records the int64 count value on the metrics recorder provided.
+func (h Int64CountHandle) Record(recorder MetricsRecorder, incr int64, labels ...string) {


Should these just be passed around by pointer everywhere? Why not?

Sure we could. This is a copy of a that x bit pointer but pointer works too.

dfawley · 2024-07-10T20:30:10Z

experimental/stats/stats.go

+// Package stats contains experimental metrics/stats API's.
+package stats
+
+// MetricsRecorder records on metrics derived from instrument registry.


We don't seem to use "instrument" anymore right?

Also I think this should be in "metrics.go" or "metricregistry.go"?

Ah whoops missed this file (I went on a crusade to switch all instances in other files). Switched.

Let's leave this here for now until we wrap up discussion on whether metrics.go should be experimental or not.

dfawley · 2024-07-10T20:32:46Z

experimental/stats/stats.go

+type MetricsRecorder interface {
+	// RecordIntCount records the measurement alongside labels on the int count
+	// associated with the provided handle.
+	RecordIntCount(Int64CountHandle, int64, ...string)


It's nice to name your parameters as a documentation aide.

Ah, yeah I was wondering this too. Named the parameters handle, incr, and labels.

dfawley · 2024-07-10T20:33:49Z

stats/metrics.go

+import "maps"
+
+// Metric is an identifier for a metric.
+type Metric string


Don't we want all these to be in experimental for now, too?

The stats package is entirely experimental as per top level package comment. I think it's fine to leave this here, since configures unstable OTel which lives in this package too.

If you're going with that philosophy then why have experimental/stats for the other stuff? Let's keep it together.

since configures unstable OTel which lives in this package too

OTel should not have anything in the stats package, right?

K will move. I guess now I can move the MetricsRecorder interface to this experimental/stats/metrics.go file as well.

dfawley · 2024-07-10T20:34:42Z

stats/metrics.go

+// Do not construct directly; use NewMetrics instead.
+type Metrics struct {
+	// Metrics are the set of Metrics to initialize.
+	Metrics map[Metric]bool


We should hide fields to prevent direct accesses, I think.

This field is read at OTel creation time. I could add a to slice function which would solve the function body comment below, but then it wouldn't be constant accesses at otel creation time when determining whether to build a metric or not. Let me see what looks best.

I added a getter for this an unexported so we can use that map to check for presence in constant time.

dfawley · 2024-07-10T20:36:00Z

stats/metrics.go

+
+// Join joins the metrics passed in with the metrics set, and returns a new copy
+// with the merged metrics.
+func (m *Metrics) Join(metrics *Metrics) *Metrics { // Use this API...


Can this function body be simplified to m.Add(m.Values()/*or whatever*/)?

I left it as is since I added a getter for map only not to convert to slice.

dfawley · 2024-07-11T15:27:48Z

experimental/stats/metricregistry.go

+
+// MetricDescriptor is the data for a registered metric.
+type MetricDescriptor struct {
+	// The name. This name must be unique across whole binary (including any per


"The name...of this metric"?

And "across the whole binary" reads a bit better.

Done for both.

dfawley · 2024-07-11T15:28:10Z

experimental/stats/metricregistry.go

+// DefaultMetrics are the default metrics registered through global metrics
+// registry. This is written to at initialization time only, and is read only
+// after initialization.
+var DefaultMetrics = stats.NewMetrics() // loop through this set,, export metrisdescriptor passed upward for pointer and labels/optional lables...


Delete comment?

Sorry missed this and a few others.

dfawley · 2024-07-11T15:29:10Z

experimental/stats/metricregistry.go

+// is passed at the recording point in order to know which metric to record
+// on.
+type Int64CountHandle struct {
+	MetricDescriptor *MetricDescriptor


Maybe you can point to a playground link illustrating the things you tried? The code as-is is still a struct so I can't see what you did.

dfawley · 2024-07-11T15:30:05Z

experimental/stats/metricregistry.go

+// GetMetric returns the MetricDescriptor from the global registry.
+//
+// Returns nil if MetricDescriptor not present.
+func GetMetric(metric stats.Metric) *MetricDescriptor {


GetMetricDescriptor maybe?

Or DescriptorForMetric?

Chose DescriptorForMetric.

zasweq · 2024-07-11T22:26:48Z

Thanks for your help; got to all comments and fixed tests.

dfawley · 2024-07-12T16:08:09Z

experimental/stats/metricregistry.go

+	// Whether this metric is on by default.
+	Default bool


Something to think about for later when you're implementing the things that actually use all of this: should we remove this field from the descriptor since it (I think?) only affects the initial registration and whether to put it into the DefaultMetrics set?

Yeah I've thought about this persistence before, I wanted to delete this since this information is encoded in DefaultSet, but this is how the user registers through API now (since we persist a copy of what user sets). Do you see a way to delete the field but keep the API?

Two obvious other options:

RegisterFooMetric() and RegisterDefaultFooMetric()

RegisterFooMetric(md MetricDescriptor, default bool)

I think this is okay, too, but we should consider the above since this data is effectively discarded immediately after registration.

I'd rather do 2. Let's keep this in mind for the future since I'm sure this metrics registry will iterate organically as we merge more layers.

dfawley · 2024-07-12T16:10:19Z

experimental/stats/metricregistry.go

+type Int64CountHandle MetricDescriptor
+
+// Record records the int64 count value on the metrics recorder provided.
+func (h *Int64CountHandle) Record(recorder MetricsRecorder, incr int64, labels ...string) {
+	recorder.RecordIntCount(h, incr, labels...)


Why does Int64CountHandle have a size on it but the RecordIntCount method does not? It seems like either both should say 64, or neither should.

Ah yeah good point. I'd prefer leaving the 64 bit identifier as OpenTelemetry and Java/C specify 64. This is because OpenTelemetry has both int/float(32||64) instruments so if we scale this registry up in the future it more aligns with OpenTelemetry.

dfawley · 2024-07-12T16:11:56Z

experimental/stats/metricregistry.go

+// NOTE: this function must only be called during initialization time (i.e. in
+// an init() function), and is not thread-safe. If multiple metrics are
+// registered with the same name, this function will panic.
+func RegisterInt64Count(descriptor MetricDescriptor) *Int64CountHandle {


Let's make sure the "numbers vs. no numbers" matches here too.

See above comment; chose numbers route.

dfawley · 2024-07-12T16:13:12Z

experimental/stats/metricregistry.go

+// MetricType is the type of metric.
+type MetricType int
+
+const (
+	MetricTypeIntCount MetricType = iota
+	MetricTypeFloatCount
+	MetricTypeIntHisto
+	MetricTypeFloatHisto
+	MetricTypeIntGauge
+)


Nit: this should maybe move to immediately before or after the MetricDescriptor type definition.

Good point. Chose after since Type MetricType field is at the last field.

dfawley · 2024-07-12T16:13:52Z

experimental/stats/metricregistry_test.go

+	}
+	RegisterInt64Count(desc)
+	RegisterInt64Gauge(desc)
+}


This test doesn't fail if a panic doesn't occur.

recover() returns nil if the test is not panicing. Then I typecast to string, and that won't contain the want right? Will my typecast of a nil to a string error?

Oh, duh, the defer will still run, sorry!

dfawley · 2024-07-12T16:15:05Z

experimental/stats/metrics.go

+	}
+}
+
+// Metrics returns the metrics set.


State that the returned map should be read-only and must not be modified, please.

Done. It's weird since it says metrics set and returned map but I think those are aliases of each other, and functionally this does return a map, which is treated as a set.

dfawley · 2024-07-12T16:16:04Z

This should be good to go modulo the small nits/test issue. Let's be ready to iterate on the registry stuff if you find it isn't working well while implementing the things that use it.

zasweq requested a review from dfawley June 25, 2024 00:46

zasweq assigned dfawley Jun 25, 2024

zasweq added the Type: Feature New features or improvements in behavior label Jun 25, 2024

zasweq added this to the 1.66 Release milestone Jun 25, 2024

zasweq force-pushed the instrument-registry branch from 0475578 to 7f9a5c3 Compare June 25, 2024 00:54

Add instrument registry

a845ca6

zasweq force-pushed the instrument-registry branch from 7f9a5c3 to a845ca6 Compare June 25, 2024 01:02

dfawley assigned zasweq and unassigned dfawley Jun 25, 2024

Move symbols around

6920f1d

zasweq assigned dfawley and unassigned zasweq Jun 26, 2024

Move Metrics Set to stats and export

e5d3d68

dfawley reviewed Jul 1, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 1, 2024

zasweq added 3 commits July 3, 2024 17:56

Snapshot for 1:1

c5cb641

Implemented offline discussion

7c051c0

Implemented offline discussion

97b48c1

zasweq force-pushed the instrument-registry branch from 9ebf079 to 97b48c1 Compare July 9, 2024 23:21

zasweq assigned dfawley and unassigned zasweq Jul 9, 2024

dfawley reviewed Jul 10, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 10, 2024

Responded to Doug's comments

94caf39

dfawley changed the title ~~internal/instrumentregistry: Add instrument registry~~ stats: Add metrics registry Jul 11, 2024

dfawley reviewed Jul 11, 2024

View reviewed changes

zasweq added 3 commits July 11, 2024 17:26

Responded to some of Doug's comments

94bc27b

Move metrics and metrics recorder to experimental/stats/metrics.go

e413a80

Fix tests and switch handles to MetricDescriptor aliases

e47eccc

zasweq assigned dfawley and unassigned zasweq Jul 11, 2024

dfawley approved these changes Jul 12, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 12, 2024

zasweq added 2 commits July 12, 2024 14:01

Responded to most of Doug's comments

83d5a1d

Added nil check for panic

6f81040

dfawley approved these changes Jul 12, 2024

View reviewed changes

switch to fmt.Sprint

1ec4e54

zasweq changed the title ~~stats: Add metrics registry~~ experimental/stats: Add metrics registry Jul 12, 2024

zasweq merged commit ecbb837 into grpc:master Jul 12, 2024
13 checks passed

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

experimental/stats: Add metrics registry (grpc#7349)

d3a0261

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

experimental/stats: Add metrics registry (grpc#7349)

9ce5163

		// Metrics are the set of Metrics to initialize.
		Metrics map[Metric]bool


		import "google.golang.org/grpc/stats"

		// Int64CountHandle is a typed handle for a int count instrument. This handle is

experimental/stats: Add metrics registry #7349

experimental/stats: Add metrics registry #7349

Conversation

zasweq commented Jun 25, 2024 • edited Loading

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

zasweq commented Jun 26, 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

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

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

dfawley left a 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

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

Choose a reason for hiding this comment

zasweq Jul 11, 2024 • edited Loading

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

zasweq Jul 11, 2024 • edited Loading

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

zasweq commented Jul 11, 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

zasweq commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

zasweq Jul 11, 2024 •

edited

Loading

zasweq Jul 11, 2024 •

edited

Loading