Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

experimental/stats: Add metrics registry #7349

Merged
merged 13 commits into from
Jul 12, 2024
61 changes: 61 additions & 0 deletions experimental/stats/instrumentregistry/instrument_registry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

// Package instrumentregistry contains the experimental instrument registry data
// structures exposed to users.
package instrumentregistry

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

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

Choose a reason for hiding this comment

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

*an int - throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// passed at the recording point in order to know which instrument to record on.
type Int64CountHandle struct {
Index int
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

}

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

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

// Float64HistoHandle is a typed handle for a float histogram instrument. This
// handle is passed at the recording point in order to know which instrument to
// record on.
type Float64HistoHandle struct {
Index int
}

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

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

49 changes: 49 additions & 0 deletions experimental/stats/stats.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

// Package stats contains experimental metrics/stats API's.
package stats

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

// MetricsRecorder records on metrics derived from instrument registry.
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

type MetricsRecorder interface {
// RecordIntCount records the measurement alongside labels on the int count
// associated with the provided handle.
RecordIntCount(instrumentregistry.Int64CountHandle, []Label, []Label, int64)
// RecordFloatCount records the measurement alongside labels on the float count
// associated with the provided handle.
RecordFloatCount(instrumentregistry.Float64CountHandle, []Label, []Label, float64)
// RecordIntHisto records the measurement alongside labels on the int histo
// associated with the provided handle.
RecordIntHisto(instrumentregistry.Int64HistoHandle, []Label, []Label, int64)
// RecordFloatHisto records the measurement alongside labels on the float
// histo associated with the provided handle.
RecordFloatHisto(instrumentregistry.Float64CountHandle, []Label, []Label, float64)
// RecordIntGauge records the measurement alongside labels on the int gauge
// associated with the provided handle.
RecordIntGauge(instrumentregistry.Int64GaugeHandle, []Label, []Label, int64)
}

// Label represents a string attribute/label to attach to metrics.
type Label struct {
// Key is the key of the label.
Key string
// Value is the value of the label.
Value string
}
199 changes: 199 additions & 0 deletions internal/stats/instrumentregistry/instrument_registry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

// Package instrumentregistry is a instrument registry that gRPC components can
Copy link
Member

Choose a reason for hiding this comment

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

*an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// use to register instruments (metrics).
package instrumentregistry

import (
"log"

"google.golang.org/grpc/experimental/stats/instrumentregistry"
"google.golang.org/grpc/stats"
)

// InstrumentDescriptor is a data of a registered instrument (metric).
Copy link
Member

Choose a reason for hiding this comment

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

"the data for"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched.

type InstrumentDescriptor struct {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// Name is the name of this metric.
Name string
// Description is the description of this metric.
Description string
// Unit is the unit of this metric.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Unit string
// Labels are the required label keys for this metric.
Labels []string
// OptionalLabels are the optional label keys for this
// metric.
OptionalLabels []string
// Default is whether this metric is on by default.
Default bool
}

// All of these globals are written to at initialization time only, and are read
// only after initialization.

// Int64CountInsts is information about registered int 64 count instruments in
// order of registration.
var Int64CountInsts []InstrumentDescriptor

// Float64CountInsts is information about registered float 64 count instruments
// in order of registration.
var Float64CountInsts []InstrumentDescriptor

// Int64HistoInsts is information about registered int 64 histo instruments in
// order of registration.
var Int64HistoInsts []InstrumentDescriptor

// Float64HistoInsts is information about registered float 64 histo instruments
// in order of registration.
var Float64HistoInsts []InstrumentDescriptor

// Int64GaugeInsts is information about registered int 64 gauge instruments in
// order of registration.
var Int64GaugeInsts []InstrumentDescriptor

// registeredInsts are the registered instrument descriptor names.
var registeredInsts = make(map[string]bool)

// ClearInstrumentRegistryForTesting clears the instrument registry for testing
// purposes only.
func ClearInstrumentRegistryForTesting() {
Int64CountInsts = nil
Float64CountInsts = nil
Int64HistoInsts = nil
Float64HistoInsts = nil
Int64GaugeInsts = nil
registeredInsts = make(map[string]bool)
instrumentregistry.DefaultMetrics = make(map[stats.Metric]bool)
}

func registerInst(name string, def bool) {
if registeredInsts[name] {
log.Panicf("instrument %v already registered", name)
}
registeredInsts[name] = true
if def {
instrumentregistry.DefaultMetrics[stats.Metric(name)] = true

Check warning on line 91 in internal/stats/instrumentregistry/instrument_registry.go

View check run for this annotation

Codecov / codecov/patch

internal/stats/instrumentregistry/instrument_registry.go#L91

Added line #L91 was not covered by tests
}
}

// RegisterInt64Count registers the int count instrument description onto the
// global registry. It returns a typed handle to use when recording data.
//
// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

How about having the user pass the InstrumentDescriptor directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

registerInst(name, def)
Int64CountInsts = append(Int64CountInsts, InstrumentDescriptor{
Name: name,
Description: desc,
Unit: unit,
Labels: labels,
OptionalLabels: optionalLabels,
Default: def,
})
return instrumentregistry.Int64CountHandle{
Index: len(Int64CountInsts) - 1,
}
}

// RegisterFloat64Count registers the float count instrument description onto the
// global registry. It returns a typed handle to use when recording data.
//
// 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 RegisterFloat64Count(name string, desc string, unit string, labels []string, optionalLabels []string, def bool) instrumentregistry.Float64CountHandle {
registerInst(name, def)
Float64CountInsts = append(Float64CountInsts, InstrumentDescriptor{
Name: name,
Description: desc,
Unit: unit,
Labels: labels,
OptionalLabels: optionalLabels,
Default: def,
})
return instrumentregistry.Float64CountHandle{
Index: len(Float64CountInsts) - 1,
}
}

// RegisterInt64Histo registers the int histogram instrument description onto the
// global registry. It returns a typed handle to use when recording data.
//
// 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 RegisterInt64Histo(name string, desc string, unit string, labels []string, optionalLabels []string, def bool) instrumentregistry.Int64HistoHandle {
registerInst(name, def)
Int64HistoInsts = append(Int64HistoInsts, InstrumentDescriptor{
Name: name,
Description: desc,
Unit: unit,
Labels: labels,
OptionalLabels: optionalLabels,
Default: def,
})
return instrumentregistry.Int64HistoHandle{
Index: len(Int64HistoInsts) - 1,
}
}

// RegisterFloat64Histo registers the float histogram instrument description
// onto the global registry. It returns a typed handle to use when recording
// data.
//
// 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 RegisterFloat64Histo(name string, desc string, unit string, labels []string, optionalLabels []string, def bool) instrumentregistry.Float64HistoHandle {
registerInst(name, def)
Float64HistoInsts = append(Float64HistoInsts, InstrumentDescriptor{
Name: name,
Description: desc,
Unit: unit,
Labels: labels,
OptionalLabels: optionalLabels,
Default: def,
})
return instrumentregistry.Float64HistoHandle{
Index: len(Float64HistoInsts) - 1,
}
}

// RegisterInt64Gauge registers the int gauge instrument description onto the
// global registry. It returns a typed handle to use when recording data.
//
// 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 RegisterInt64Gauge(name string, desc string, unit string, labels []string, optionalLabels []string, def bool) instrumentregistry.Int64GaugeHandle {
registerInst(name, def)
Int64GaugeInsts = append(Int64GaugeInsts, InstrumentDescriptor{
Name: name,
Description: desc,
Unit: unit,
Labels: labels,
OptionalLabels: optionalLabels,
Default: def,
})
return instrumentregistry.Int64GaugeHandle{
Index: len(Int64GaugeInsts) - 1,
}
}
Loading