Golang metrics prototype #100

jmacd · 2019-08-16T20:17:52Z

This satisfies the changes described in RFC 0003, and 0009.
This does not incorporate metrics observers (RFC 0008).

This includes the proposed LabelSet changes from open-telemetry/oteps#49

akehlenbeck · 2019-08-19T13:56:06Z

api/metric/api.go

@@ -22,60 +22,156 @@ import (
 	"go.opentelemetry.io/api/unit"
 )

-type MetricType int
+type Type int


Minor suggestion from the peanut gallery: pick a word other that "type" for this. GCP uses "kind" so there's maybe some prior art for using that terminology. The word "type" can then be used exclusively to refer to int/float/etc (the value type) and "kind" can refer to the orthogonal idea of how to interpret those values.

Thanks, good suggestion.

krnowak

Some comments and ideas.

krnowak · 2019-08-20T11:06:30Z

api/metric/api.go

+	return ts.Variable.Defined()
+}
+
+func (ts TimeSeries) Description() TimeSeries {


What's the point of this function? If it's to implement the Metric interface, then maybe add the var _ Metric = TimeSeries{} somewhere.

This was removed.

krnowak · 2019-08-20T11:10:17Z

api/metric/api.go

-type Option func(*Handle, *[]registry.Option)
+type Measurement struct {
+	Metric Metric
+	Value  float64


There seems to be two types of fields, float and int. Should this struct also contain an int64 field then? The Metrics field would be the discriminator I think.

In other languages, I've seen the SDK convert int->float in the exporter. If we must distinguish these types and not convert int->float->int, this will have to change.

krnowak · 2019-08-20T11:46:08Z

example/basic/main.go

@@ -69,6 +71,25 @@ func main() {

 		gauge.Set(ctx, 1)

+		meter.RecordBatch(ctx,
+			// TODO: Determine how the call-site should look for batch recording.


I'd add some helpers in the metric package, like:

meter.RecordBatch(ctx, metric.Float64Measurement(oneMetric, 1.0, anotherKey.String("xyz")), metric.Int64Measurement(measureTwo, 42, anotherKey.String("zyx")), )

(This assumes that the Measurement should have a separate field for int64 metrics.)

In OpenCensus there was a syntax like

meter.RecordBatch(ctx, oneMetric.M(1.0), measureTwo.M(42))

We've discussed dropping call-site labels from the spec, so anotherKey.String("xyz") disappears from the example.

krnowak · 2019-08-20T13:41:34Z

experimental/streaming/exporter/reader/format/format.go

+	case observer.BATCH_METRIC:
+		buf.WriteString("BATCH ")
+		for _, m := range data.Measurements {
+			// TODO: This breaks down because Gauge supports more than


Looks like Measurement is ambiguous in this case. Just an idea: should metric.Measurement should be an interface and gets returned by the implementations of the Float64Gauge and others interfaces? So the Float64Gauge interface would have two more functions like:

SetMeasurement(ctx context.Context, value float64, labels ...core.KeyValue) Measurement AddMeasurement(ctx context.Context, value float64, labels ...core.KeyValue) Measurement

or we can keep Measurement as a concrete type (we seem to have a problem with interfacing all the things) and then add another enum:

type MeasurementKind int const ( F64GaugeAdd MeasurementKind = iota F64GaugeSet F64CumulativeInc F64MeasureRecord … // same for I64 )

and Measurement would have an additional Kind field. Then we wouldn't need to add anything to the Float64Gauge interface and others.

With that change, maybe we won't need both SET_METRIC and UPDATE_METRIC. That information would already be a part of the Measurement object.

Done (and agreed in our working session).

I forgot to add that there was some discussion about this, and a decision to support only MeasureMetric updates in the RecordBatch API. As it stands in the PR, any metric could be passed, which I actually prefer, but it's something we could change later.

krnowak · 2019-08-20T13:45:31Z

experimental/streaming/exporter/reader/format/format.go

+	buf.WriteString(" {")
+	i := 0
+	if m.Tags != nil {
+		// Note: This if-conditional can be removed after Map is not


Just need another approver to review #89. This goes a bit slowly…

cep21 · 2019-08-27T00:23:17Z

Hi,

I understand this is marked as "do not review", but I would caution against the implemented Meter interface. It has a large number of methods. Go works best when interfaces have very few methods. Most interfaces in the go standard library contain a single method. Large interfaces are generally a code smell.

There exist alternative implementations that do not need such large interfaces. This talk at Gophercon 2016 talks about a similar thought process of how the random package in go can be broken down from a large interface into a smaller one.

jmacd · 2019-08-27T22:11:39Z

Thanks, @cep21. I think I agree with your point in general, but the metrics interface is simply quite complicated. If you compare with the Prometheus Go interface,

https://godoc.org/github.com/prometheus/client_golang/prometheus

I think we've been able to reduce complexity without losing the optimization potential which is such a key requirement for OpenTelemetry--to match the Prometheus implementation for performance.

There are potential simplifications. Instead of 3 methods for float64 metrics and 3 methods for int64 metrics, we could have just 3 and use adapters to support int64 metrics. Or we could not support int64 metrics (I would agree to this, but the spec says to support both).

That said, the purpose of these interfaces is to have a separation between the API and the implementation. In this case, I opted for separate int64 and float64 metric interface methods because I believe there is someone out there who will argue that the API should not impose an int64->float64 conversion on the implementation. So, this interface is not trying to achieve the best Go interface in the traditional sense, it's trying to achieve the most complete separation of API and implementation possible.

Happy to hear if you have specific improvements in mind. I think the #metrics-specs channel in Gitter would be a good place to discuss this. Thanks.

cep21 · 2019-08-27T22:50:40Z

but the metrics interface is simply quite complicated

I think the rand example I linked above is a good way to resolve this. I'm comparing it to interfaces I've written in the past. The closest thing available right now that is similar is the interface of https://github.com/segmentio/stats/blob/master/handler.go#L15. I believe with the right concrete abstractions on top of this interface opentelementry-go can achieve everything it needs and have an API that is a single method.

reduce complexity without losing the optimization potential

It will also be more efficient than the currently proposed API in terms of fewer global locks and less memory allocation per method, if a few tricks are used.

I think the #metrics-specs channel in Gitter would be a good place to discuss this

I'm worried a chat room is the wrong place for long form proposals. Is there a documentation format that is preferred.

jmacd

Please take another look @krnowak @cep21, thanks.

jmacd · 2019-08-30T08:31:58Z

api/metric/api.go

-type Option func(*Handle, *[]registry.Option)
+type Measurement struct {
+	Metric Metric
+	Value  float64


In other languages, I've seen the SDK convert int->float in the exporter. If we must distinguish these types and not convert int->float->int, this will have to change.

jmacd · 2019-08-30T08:35:47Z

This is tracking open-telemetry/oteps#29

krnowak

Some nits and questions.

krnowak · 2019-08-30T11:10:36Z

api/metric/api.go

+}
+
+// Option supports specifying the various metric options.
+type Option func(*Instrument, *[]registry.Option)


I find this prototype kind of icky, because it gives you an access to whole Instrument object, which allows your Option implementation to change anything it wants there, only to find out that the Kind and Variable fields of the Instrument are clobbered by the registerInstrument function…

I wonder if it wouldn't be better to have the usual Options struct:

type Options struct { Desc string // will be translated to registry.WithDescription(desc) Unit unit.Unit // will be translated to registry.WithUnit(unit) Keys []core.Keys // will become Keys field of the instrument }

I'm all in favor of using an options struct here. I will update later in this PR, since the spec says to add options for positive-only, etc.

krnowak · 2019-08-30T11:20:54Z

example/basic/main.go

-		oneMetric,
-		lemonsKey.Int(10),
-	)
+	gauge := oneMetric.Handle(ctx, meter, lemonsKey.Int(10))


Shouldn't this also use fooKey and barKey? Or maybe it would be good to describe a comment what happens here if those keys are missing.

Good catch. I need to update the RFC about this topic, it was discussed heavily in the working session.

krnowak · 2019-08-30T11:21:32Z

example/basic/main.go

-	)
+	gauge := oneMetric.Handle(ctx, meter, lemonsKey.Int(10))
+
+	measure := measureTwo.Handle(ctx, meter, lemonsKey.Int(10))


Similar situation here - the lemonsKey is going to be ignored, because is not a part of predefined keys, right?

I'll update the RFC, this requires detail.

krnowak · 2019-08-30T11:23:15Z

example/basic/main.go

+			tag.NewContext(ctx,
+				tag.Insert(anotherKey.String("xyz"))),
+
+			metric.Measurement{


Hm, is there no possibility to define values for the predefined keys when doing batched measurements?

The handles contain the pre-defined label values. There's no ability to set additional labels on the call site, as agreed to in the working session.

krnowak · 2019-08-30T11:24:20Z

experimental/streaming/exporter/observer/observer.go

-	Parent  ScopeID // START_SPAN
-	Stats   []stats.Measurement
-	Stat    stats.Measurement
+	String       string  // START_SPAN, EVENT, ...


Looks like SET_NAME disappeared here.

krnowak · 2019-08-30T11:26:58Z

experimental/streaming/exporter/reader/format/format.go

+	buf.WriteString(fmt.Sprint(m.Value))
+	buf.WriteString(" {")
+	i := 0
+	if m.Tags.Len() != 0 {


m.Tags.Foreach should work perfectly fine on an empty map, so this if condition can be now removed.

(Yeah, it was a nil interface before)

krnowak · 2019-08-30T11:28:39Z

experimental/streaming/exporter/reader/reader.go

-			read.SpanContext = span.spanContext
+		if event.Context != nil {
+			span := trace.CurrentSpan(event.Context)
+			if span != nil {


Should we warn about /ignore the event if the span is nil? The spanlog reporter is going to panic in this situation…

We should treat the metric event as though it has no span context, otherwise it should be OK

krnowak · 2019-08-30T11:28:52Z

experimental/streaming/exporter/reader/reader.go

+
+		if event.Context != nil {
+			span := trace.CurrentSpan(event.Context)
+			if span != nil {


Same here, I think.

Yeah, just a nil check looks good. If the reader will panic because SpanContext isn't set, let's fix that.

krnowak · 2019-08-30T11:30:18Z

experimental/streaming/exporter/spandata/spandata.go

 		for _, r := range s.readers {
 			r.Read(span)
 		}
 		delete(s.spans, data.SpanContext)
 	}
 }
+
+func (s *spanReader) updateMetric(data reader.Event) {
+	// TODO aggregate


It would be great to have this addressed before the PR is merged. Maybe this will uncover some problems with the API?

Right. I was planning to attach some other metrics libraries instead.
We're also looking to port over the OpenCensus metrics exporter, which will satisfy this TODO.

I have the start of an SDK based on several-changes-ago of this branch, which I'll update today, and then we'll have a better look at the big picture.

krnowak · 2019-08-30T11:30:56Z

experimental/streaming/sdk/metric.go

+	scope      observer.ScopeID
+}
+
+func (h *metricHandle) RecordAt(ctx context.Context, t time.Time, value float64) {


t time.Time parameter seems to be ignored. Should we set the Time field of the Event with it?

Actually, what's the meaning of the time.Time parameter? I see it can be set for each record when recording metrics one by one, but it can't be set for the batch updates at all. Should batch updates have a timestamp for each measurement or one for all of them or none at all (as it is currently)?

I'm removing it. It's something we could specify later. I'm not sure how important users will find the ability to provide custom timestamps on metrics. We have custom timestamps on spans, but it's less clear if this is needed for metrics.

I'm not sure how important users will find the ability to provide custom timestamps on metrics.

The primary motivation is that time is a critical part of time series data, so the core abstract should contain it even if the layers above do not. In the past I've used the zero value of time.Time to imply "real time". It could be possible people have their own buffering above open telemetry and want to use the timestamp of values when they enter that, not when they enter open telemetry.

cep21 · 2019-08-30T18:01:36Z

api/metric/api.go

-type Handle struct {
+// Instrument represents a named metric with recommended local-aggregation keys.
+type Instrument struct {
+	// Variable defines the metric name, description, and unit.
 	Variable registry.Variable


Why is description and kind part of the key?

This could maybe use some work. There is a common Variable type which represents metrics and keys, since they both are named things which we're interested in describing.

There are some open questions about registration of key and metric names, to avoid passing around full structs. Is that what you're thinking? I believe there is a desire to allow metric instruments to be allocated as vars in the compilation unit where they are used. As it stands, metrics and keys are not bound to the SDK, so that they do not need a SDK to initialize. They can initialize before any SDK exists, which is part of the reason why the Registry isn't well defined yet.

For my interest in a streaming SDK, I would be happy to record an event with a unique ID assigned to each key or metric, along with metadata like the description, and then at runtime the unique ID would stand in for the string that was registered. So, I'd like all keys and metric names to be assigned unique IDs, but don't want a registry to try and maintain a map of all keys/metrics in the process.

cep21 · 2019-08-30T18:03:35Z

api/metric/api.go

+// Recorder is the implementation-level interface to Set/Add/Record individual metrics.
+type Recorder interface {
+	// RecordAt allows the SDK to observe a single metric event
+	RecordAt(ctx context.Context, t time.Time, value float64)


Are you saying people will have to do

c.Inc(ctx, 1)

rather than

c.Inc(1)

Yes. The rationale is that this lets us associated the distributed context with the update, allowing us to record the update in a span or as a structured metric event with all of its labels, not only those defined in the handle.

That totally makes sense to me, but it's very strange for a function to take a context and not use the context to time out or unblock itself.

It's like "Take this thing that implies timeouts and cancelation, but don't actually use it to timeout or cancel anything. Just use it as a grabbag"

I wonder if it is better to take one of these.

type Valuer interface { Value(key interface{}) interface{} } var _ Valuer = context.Context(nil)

cep21 · 2019-08-30T18:04:15Z

api/metric/api.go

+	RecorderFor(context.Context, Instrument, ...core.KeyValue) Recorder
+
+	// RecordBatch atomically records a batch of measurements.
+	RecordBatch(context.Context, ...Measurement)


atomically may be too strict a requirement. What if the buffer can only fit 2 of them? I think it's fine to accept those two and trigger back pressure logic for the rest.

When this was discussed, it was being done explicitly to support recording inter-dependent metrics, where you wouldn't want an update to be split apart because it could create inconsistency.

I'm not sure the caller ever wants us to block. I'm starting to think this API should return an error in this sort of condition,.

If you return errors you're asking the caller to check them, which makes metric reporting verbose. It may be better to register an "onerror" callback with the system that takes measurements that fail, and the library user can decide what to do.

cep21 · 2019-08-30T18:06:15Z

api/metric/api.go

+// Meter is an interface to the metrics portion of the OpenTelemetry SDK.
+type Meter interface {
+	// RecorderFor returns a handle for observing single measurements.
+	RecorderFor(context.Context, Instrument, ...core.KeyValue) Recorder


In my APIs I invert this. ObserverAt(tsid, meta)

I'm not sure I understand. I think tsid corresponds with Instrument and meta corresponds with ...core.KeyValue. Do you prefer "At" to "For"? I stuck with "Record" as a stem because of the existing verbs in the spec, which talk about recording metrics. I could be convinced that "Observer" is as good.

In the past I've broken time series data into 4 components

Time

Value

Time series identifier (tsid)

Metadata.

Time and value are obvious. The TSID is the unique identifier that metrics aggregate upon (for example, for SignalFx or Datadog or cloudwatch this is a metric name and dimensions/tags). Metadata are extra concepts that don't change aggregation. Those are description/unit/etc.

I usually clearly separate those 4 into their own components. I think opentelemetry tries to combine tsid and metadata into the same struct. It may not be possible to split (3) and (4) in a generic way.

cep21 · 2019-08-30T18:07:20Z

api/metric/api.go

+// Handle contains a Recorder to support the implementation-defined
+// behavior of reporting a single metric with pre-determined label
+// values.
+type Handle struct {


Where are the methods for handle defined?

Handle doesn't have any methods. There are Gauge, Cumulative, and Measure handles that have the appropriate method name. The other way Handle is used is as an argument to RecordBatch.

cep21 · 2019-08-30T18:08:38Z

api/metric/api.go

-// WithKeys applies the provided dimension keys.
+// WithKeys applies recommended dimension keys.  Multiple `WithKeys`
+// options accumulate.  The keys specified in this way are taken as
+// the aggregation keys for Gauge and Cumulative.
 func WithKeys(keys ...core.Key) Option {


Why are there keys without values?

These are keys that specify which values must be specified in a handle for a metric. Instruments (Gauge, Cumulative, Measure) have a set of keys, Handles have a set of pre-defined key:values.

Some work on the RFC and terminology are needed here. These keys specify values that are always defined by a handle for this metric, values that will not be overridden by the context. Any subset of these keys may be used for efficient pre-aggregation in the SDK.

cep21 · 2019-08-30T18:08:58Z

api/metric/api.go

-		return "unknown"
-	}
+// Defined returns true when the instrument has been registered.
+func (inst Instrument) Defined() bool {


Why does the Instrument need to exist?

I'm not sure what you're asking. There are two kinds of thing here, one metric instrument (which does not depend on the SDK, is a plain struct) and one metric handle (which does depend on the SDK, has pre-defined label values, contains an interface to record with).

cep21 · 2019-08-30T18:10:18Z

api/metric/cumulative.go

+	Handle
+}
+
+type Int64CumulativeHandle struct {


What's the difference between a cumulative and handle?

For example, the Int64Cumulative contains the metric name and other details. Int64CumulativeHandle associates the metric name with a set of pre-defined label values. I'll add more documentation, sorry.

cep21 · 2019-08-30T18:10:35Z

api/metric/api.go

+	RecorderFor(context.Context, Instrument, ...core.KeyValue) Recorder
+
+	// RecordBatch atomically records a batch of measurements.
+	RecordBatch(context.Context, ...Measurement)


Why does RecordBatch need to exist if you can create it from RecorderFor

I once proposed we remove this interface,
https://github.com/open-telemetry/oteps/blob/6fc43532a8acc9024c143fa85777d53d2e646208/0003-eliminate-stats-record.md

The argument in favor of keeping it was that about inter-dependent metrics. E.g., gRPC would like to record N statistics at the end of an RPC, but they want to be sure that either 0 or N are reflected in the outcome, otherwise metrics summarization will be inaccurate.

A secondary argument is that there may be less locking overhead.

cep21 · 2019-08-30T18:26:32Z

experimental/streaming/sdk/package.go

+	metric.Meter
+}
+
+var _ SDK = (*sdk)(nil)


People normally write this

var _ SDK = &sdk{}

I feel like I've seen (*T)(nil) more often. It is the styled presented in Effective Go.

You're totally right about that. Seeing * and nil together somewhat hurts my brain. It's unclear why that's a preferred style over &sdk{}.

cep21 · 2019-09-04T16:54:10Z

example/basic/main.go

@@ -44,7 +46,7 @@ var (
 		metric.WithDescription("A gauge set to 1.0"),
 	)

-	measureTwo = stats.NewMeasure("ex.com/two")
+	measureTwo = metric.NewFloat64Measure("ex.com/two")
 )

 func main() {


I understand there is a main.go example, but it would help to use https://blog.golang.org/examples for each individual concept to make it clear which are the minimal parts for each thing it's trying to do. With the main.go approach it's unclear which aspects are needed for which example it's trying to give. Like:

Minimal example of a counter

Minimal example of having a dimension on a metric

Minimal example of aggregating.

Maybe an example is

func ExampleGauge() { oneMetric = metric.NewFloat64Gauge("ex.com/one", metric.WithKeys(fooKey, barKey, lemonsKey)) gauge := oneMetric.Handle(ctx, meter, lemonsKey.Int(10)) gauge.Set(1) }

Add a // Output: to the end of your example and it becomes code that is also executed when you run go test

Another advantage of testable examples is that they usually integrate with an IDE and show up on help dialogs.

This is a good suggestion, I always like the examples.

api/metric/api.go

experimental/streaming/exporter/reader/format/format.go

jmacd · 2019-09-13T23:39:07Z

This is now tracking the LabelSet proposal here:
open-telemetry/oteps#49

paivagustavo · 2019-09-14T02:35:02Z

api/metric/api.go

+	// Kind is the metric kind of this instrument.
+	Kind Kind
+
+	// Bidirectional implies this is an up-down Cumulative.


What is the use case for Bidirectional and Unidirectional? Do we need both variables or should this be changed to a single variable? if I get it correctly, maybe it could be renamed to Monotonic

Ideally (I think) there would be some kind of union discriminated by the metric kind. So if kind were Cumulative (or soon-to-be-Counter), then the only accessible member would be Bidirectional. If kind were Gauge then you could only access Unidirectional. Same goes for the Measure kind and the NonNegative field. But this is Go and you don't have it, so that's why this struct looks a bit like reflect.Type interface, where docs say that the function will panic if used with wrong Kind. Which maybe would be nice to follow here too - have Bidirectional, Unidirectional and NonNegative as function that check if the kind is Cumulative, Gauge or Measure, respectively and panic if it isn't?

Use cases? These are options that enable less common (I suppose) kinds of metrics - usually Cumulative only grows, but maybe there are some metrics where it is possible for it to shrink. Similar goes to Gauge being usually random and Measure being usually >= 0.

This is a great point, and it belongs in the RFC 0003 discussion.. Please use this PR: open-telemetry/oteps#51

I like the idea of replacing "Unidirectional" with "Monotonic": this option applies to Gauges and is not the default behavior.

For Counters, Monotonic is the default behavior, so we still need a name for the option to support bi-directional counters. I'm not aware of a good opposite for monotonic. Do you have any suggestions?

Sent the discussion to mentioned PR.

@krnowak I suppose the Option objects could be made specific to the particular constructor, so that NewCounter(..., NonMononic()) would build but NewCounter(..., NonNegative) would not.

Also FYI I updated the RFCs to use "Monotonic" but we're still working out whether "NonMonotonic" is a preferred to "BiDirectional".

krnowak

Some more comments. What's missing still is docs. Especially those explaining the interplay between the , Handle, Instrument and LabelSet types.

krnowak · 2019-09-16T19:14:58Z

Makefile

 	$(GOTEST) $(GOTEST_OPT) $(ALL_PKGS)

+.PHONY: examples
+examples:
+	@for ex in $(EXAMPLES); do (echo $${ex} && cd $$(dirname $${ex}) && go build $$(basename $${ex})); done


Would @for ex in $(examples); do go build $$(dirname $${ex}); done work too?

krnowak · 2019-09-16T19:26:44Z

api/metric/api.go

 type Meter interface {
-	// TODO more Metric types
-	GetFloat64Gauge(ctx context.Context, gauge *Float64GaugeHandle, labels ...core.KeyValue) Float64Gauge
+	// DefineLabels returns a reference to a set of labels that


This comment is confusing me. The application sets the labels, so it already knows what are those. So I'm not sure what "set of labels that cannot be read by the application" is supposed to mean.

I meant that you cannot read the labels from a LabelSet. I don't think you would need to, but the point is that the SDK is not required to keep this information, it could potentially stream it and forget--retain only an identifier.

krnowak · 2019-09-16T19:50:40Z

api/metric/api.go

+	// Kind is the metric kind of this instrument.
+	Kind Kind
+
+	// Bidirectional implies this is an up-down Cumulative.


Ideally (I think) there would be some kind of union discriminated by the metric kind. So if kind were Cumulative (or soon-to-be-Counter), then the only accessible member would be Bidirectional. If kind were Gauge then you could only access Unidirectional. Same goes for the Measure kind and the NonNegative field. But this is Go and you don't have it, so that's why this struct looks a bit like reflect.Type interface, where docs say that the function will panic if used with wrong Kind. Which maybe would be nice to follow here too - have Bidirectional, Unidirectional and NonNegative as function that check if the kind is Cumulative, Gauge or Measure, respectively and panic if it isn't?

Use cases? These are options that enable less common (I suppose) kinds of metrics - usually Cumulative only grows, but maybe there are some metrics where it is possible for it to shrink. Similar goes to Gauge being usually random and Measure being usually >= 0.

krnowak · 2019-09-16T20:02:11Z

api/metric/common.go

 )

-func registerMetric(name string, mtype MetricType, opts []Option, metric *Handle) {
-	var varOpts []registry.Option
+func registerInstrument(name string, kind Kind, opts []Option, inst *Instrument) {


Should the function check bogus set ups like NonNegative for GaugeKind and warn/panic in such cases? That way it would be easier to spot when we did something wrong.

bogdandrutu · 2019-09-18T20:14:08Z

api/metric/api.go

+type InstrumentID uint64
+
+// Instrument represents a named metric with recommended local-aggregation keys.
+type Instrument struct {


this is more like what we called in the protocol and what OpenMetrics as well calls "MetricDescriptor"

I'm open to a change of Instrument to Descriptor.

This is to shrink the PR open-telemetry#100. The only place where the registry.Variable type was used was metrics, so just inline that type into its only user. The use of the registry.Variable type in core.Key was limited to the Name field. The stats package also used the registry.Variable type, but seems that also only the Name field was used and the package is going to be dropped anyway.

This is to shrink the PR #100. The only place where the registry.Variable type was used was metrics, so just inline that type into its only user. The use of the registry.Variable type in core.Key was limited to the Name field. The stats package also used the registry.Variable type, but seems that also only the Name field was used and the package is going to be dropped anyway.

jmacd · 2019-09-20T21:57:44Z

I believe @krnowak is going to copy this branch into a new one, so it's not forked from my repo.

krnowak · 2019-10-07T20:47:24Z

Updated. Added some docs, but with every line of the docs I wrote I felt dumber and dumber (or just tired), so I'd be grateful for the review. Certainly more of them need to be written still.

Things to do or ideas:

Add some mock implementation of metrics and add more tests.
Moar docs. Likely about various option for Descriptor in doc.go.
Maybe make Observer type an interface to be implemented by SDK - that way I could hide the Descriptor stuff and in turn you couldn't do meter.NewHandle(observer.Descriptor, labels) because that would be a compiler error.
- That would also make Descriptor of an observer inaccessible to the API users, and would be inconsistent with other metrics.
- Other idea - have a separate ObserverDescriptor struct and some helper function to convert it to Descriptor for the SDK use.
Maybe implement some atomic ops in MeasurementValue? (Not sure.)
- What would be the value of func (v *MeasurementValue) Add(raw uint64, kind ValueKind) for the SDK?

jmacd · 2019-10-07T20:58:02Z

About the Observer problem, it looks like NewHandle is really causing the issue. Maybe we could change the signature of NewHandle to take an interface, one that is implemented by the other three kinds but not by Observer. It could be a no-op method called "SupportHandle()".
All four instruments would support a Descriptor() *Descriptor method, only the three that support handles would add the second method to satisfy the interface.

jmacd · 2019-10-07T21:00:04Z

I don't feel we need to worry about atomic operations on MeasurementValues. I see them as a transfer type--the SDK will do what it needs.

jmacd · 2019-10-07T21:01:13Z

I can't approve this because I opened it. But I approve.

rghetia · 2019-10-08T17:06:28Z

api/metric/gauge.go

 func (g *Int64Gauge) Measurement(value int64) Measurement {
 	return g.int64Measurement(value)
 }

+// Set sets the value of the gauge to the passed value. The labels


Maybe rephrase it to "Set assigns the passed value to the value of the gauge".
Same for all 'Set' methods.

rghetia · 2019-10-08T17:13:39Z

api/metric/measure.go

 func (c *Float64Measure) Record(ctx context.Context, value float64, labels LabelSet) {
 	c.recordOne(ctx, NewFloat64MeasurementValue(value), labels)
 }

+// Record adds a new value to the list of measure's records. The
+// labels should contain the keys and values specified in the measure
+// with the WithKeys option.


Maybe rephrase the above to "The labels should contain the keys and values for each key specified in the measure with the WithKeys option"
What is the expected behavior if it doesn't have all the keys-value pair in the label set?

rghetia

Couple of doc comments. Otherwise LGTM.

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.29.1 to 1.30.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.29.1...v1.30.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

jmacd requested review from bogdandrutu, pjanotti, rghetia, sjkaris and tedsuo as code owners August 16, 2019 20:17

akehlenbeck reviewed Aug 19, 2019

View reviewed changes

krnowak reviewed Aug 20, 2019

View reviewed changes

jmacd added the do not review label Aug 22, 2019

jmacd commented Aug 30, 2019

View reviewed changes

jmacd removed the do not review label Aug 30, 2019

krnowak reviewed Aug 30, 2019

View reviewed changes

cep21 reviewed Aug 30, 2019

View reviewed changes

cep21 reviewed Sep 4, 2019

View reviewed changes

cbandy reviewed Sep 11, 2019

View reviewed changes

api/metric/api.go Outdated Show resolved Hide resolved

experimental/streaming/exporter/reader/format/format.go Outdated Show resolved Hide resolved

jmacd added the do not review label Sep 12, 2019

jmacd removed the do not review label Sep 13, 2019

paivagustavo reviewed Sep 14, 2019

View reviewed changes

krnowak reviewed Sep 16, 2019

View reviewed changes

paivagustavo mentioned this pull request Sep 17, 2019

WIP: Discuss approval for three metrics RFCs 0003, 0008, and 0009. open-telemetry/oteps#51

Closed

bogdandrutu reviewed Sep 18, 2019

View reviewed changes

This was referenced Sep 19, 2019

Drop the registry package #130

Merged

Build example and ensure go mod files are tidy #131

Merged

krnowak added 15 commits October 7, 2019 20:57

merge alternatives into a single bool

b519b22

make value kind name less stuttery

89f13f0

fix observation callback prototype

f405bf4

drop context parameter from NewHandle

760906f

drop useless parameter names

1c8e7e7

make descriptor an opaque struct

bfca093

use a store helper

200178e

handle comment fixes

ba4bb01

reword Alternate comment

2eb049a

drop the "any value" metrics

7805077

make measurement value simpler

1eaac50

document value stuff

cf4b0b0

add tests for values

b7fd1b5

docs

94ce64a

do not panic if there is no span ID in the event

9b9c274

krnowak force-pushed the jmacd/metrics_proposal2 branch from dbda224 to 9b9c274 Compare October 7, 2019 20:31

rghetia reviewed Oct 8, 2019

View reviewed changes

rghetia approved these changes Oct 8, 2019

View reviewed changes

Merge branch 'master' into jmacd/metrics_proposal2

c84ca30

rghetia merged commit be8fb0b into open-telemetry:master Oct 8, 2019

krnowak mentioned this pull request Oct 9, 2019

REQUEST: New membership for krnowak open-telemetry/community#213

Closed

6 tasks

jmacd deleted the jmacd/metrics_proposal2 branch October 29, 2019 21:09

jmacd mentioned this pull request Nov 4, 2019

Inconsistent API design between trace and metri #277

Closed

jmacd mentioned this pull request Nov 20, 2019

Prometheus exporter #334

Merged

c24t mentioned this pull request Nov 25, 2019

Update 0009-metric-handles to match current spec open-telemetry/oteps#62

Merged

Golang metrics prototype #100

Golang metrics prototype #100

Conversation

jmacd commented Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak 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

cep21 commented Aug 27, 2019

jmacd commented Aug 27, 2019

cep21 commented Aug 27, 2019 • edited Loading

jmacd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmacd commented Aug 30, 2019

krnowak 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

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

cep21 Sep 4, 2019 • 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

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

jmacd commented Aug 16, 2019 •

edited

Loading

cep21 commented Aug 27, 2019 •

edited

Loading

cep21 Sep 4, 2019 •

edited

Loading