Metrics proposal for Go #2

jmacd · 2019-07-19T23:59:37Z

This implements a proof-of-concept for https://github.com/open-telemetry/rfcs/pull/4}

See examples/grpc/metrics.go for how the gRPC metrics translate from
https://github.com/census-instrumentation/opencensus-specs/blob/master/stats/gRPC.md

bogdandrutu · 2019-07-22T14:11:34Z

api/metric/api.go

+	Invalid    Type = iota
+	Gauge           // Supports Set()
+	Cumulative      // Supports Inc(): only positive values
+	Additive        // Supports Add(): positive or negative


Additive and Gauge can be unified.

They could be. I was hoping to make there be a 1:1 mapping between measured value and the interpretation. If we decide to keep a Stats API (i.e., reject RFC 0003), then I feel strongly that the Metrics API should be layered on top of the Stats API, so there should be a 1:1 mapping from a metrics update into a lower-level stats measurement. If we combine Additive and Gauge, for example, then I need to know the kind of operation in order to interpret a statistic. By allowing the Additive and Gauge to be separate metric types, I can layer Metrics on top of Stats, because each Measure has only one kind. I.e., there's not a mixture of Set() and Add() calls in the same stream of measurements.

bogdandrutu · 2019-07-22T14:13:02Z

api/metric/api.go

-	Cumulative            // Supports Inc()
+	Invalid    Type = iota
+	Gauge           // Supports Set()
+	Cumulative      // Supports Inc(): only positive values


Cumulative also needs a set, because the classic example for this is CPU usage. If you want to report a CPU usage it is always going up (unless you do a CumulativeGauge that remembers the previous value and Inc delta).

bogdandrutu · 2019-07-22T14:14:23Z

api/metric/aggregation/descriptor.go

+	SUM
+	COUNT
+	MIN
+	MAX


Why min/max? Where is this used?

bogdandrutu · 2019-07-22T14:15:25Z

api/metric/aggregation/descriptor.go

+	MIN
+	MAX
+	LAST_VALUE
+	DISTRIBUTION


For a distribution if a histogram is used user requires to also configure the buckets. If a summary is used user requires to configure the percentiles. So I cannot see how this is used.

bogdandrutu · 2019-07-22T14:16:48Z

api/metric/api.go

-		return "unknown"
+// WithAggregation applies a user-recommended aggregation to this
+// metric, useful particularly for Measure type metrics.
+func WithAggregation(aggr aggregation.Operator) Option {


This is not compatible with OpenCensus stats which is a primary requirement for this, because we need to not allow the instrumentation plugin to define the aggregation.

This is not a definition. This is a recommendation. The SDK is not bound to follow this advice. If this value is not set, the default aggregation for MeasureMetrics is None. I like this because the application programmer can suggest None as a good aggregation for measure metrics, but could also suggest Distribution.

bogdandrutu · 2019-07-22T14:27:30Z

api/trace/current.go

@@ -25,10 +25,16 @@ var (
 )

 func SetCurrentSpan(ctx context.Context, span Span) context.Context {
+	if ctx == nil {


why these changes?

In my exporter, I want to know the current span in order to annotate the span data with accumulated metrics, grouped by span_id. I want this to happen for all callers, without the programmer having to do anything. So this check avoids a crash when you ask for the current span and there's no context.

bogdandrutu · 2019-07-22T14:28:21Z

api/metric/api.go

 )

 type Meter interface {
-	// TODO more Metric types
-	GetFloat64Gauge(ctx context.Context, gauge *Float64GaugeHandle, labels ...core.KeyValue) Float64Gauge
+	GetFloat64Gauge(context.Context, *Float64GaugeHandle, ...core.KeyValue) Float64Gauge


Why a context in this case?

This allows context tags to be recorded, in case the SDK wants to do that. In addition, it allows us to associate the current active span with the measurement, which means we can reflect this metric update in the recorded span data, for example.

bogdandrutu · 2019-07-22T14:28:51Z

api/metric/api.go

+type Float64Measure interface {
+	Record(ctx context.Context, value float64, labels ...core.KeyValue)
+}
+
 type Handle struct {


What is a Handle? why does this exist?

A handle is a common base type for any of the metrics. In this proposal, the only API difference between the metric types is the verb they use, the interpretation is left to the SDK. So the Handle is a thing that describes the metric independently of its verb or metric type. The properties of a metric are: variable (name), type (gauge, additive, cumulative, or measure), keys (recommended), and aggregation (recommended).

jmacd · 2019-07-22T23:08:25Z

api/metric/api.go

+	Invalid    Type = iota
+	Gauge           // Supports Set()
+	Cumulative      // Supports Inc(): only positive values
+	Additive        // Supports Add(): positive or negative


They could be. I was hoping to make there be a 1:1 mapping between measured value and the interpretation. If we decide to keep a Stats API (i.e., reject RFC 0003), then I feel strongly that the Metrics API should be layered on top of the Stats API, so there should be a 1:1 mapping from a metrics update into a lower-level stats measurement. If we combine Additive and Gauge, for example, then I need to know the kind of operation in order to interpret a statistic. By allowing the Additive and Gauge to be separate metric types, I can layer Metrics on top of Stats, because each Measure has only one kind. I.e., there's not a mixture of Set() and Add() calls in the same stream of measurements.

jmacd · 2019-07-22T23:09:15Z

api/metric/api.go

 )

 type Meter interface {
-	// TODO more Metric types
-	GetFloat64Gauge(ctx context.Context, gauge *Float64GaugeHandle, labels ...core.KeyValue) Float64Gauge
+	GetFloat64Gauge(context.Context, *Float64GaugeHandle, ...core.KeyValue) Float64Gauge


This allows context tags to be recorded, in case the SDK wants to do that. In addition, it allows us to associate the current active span with the measurement, which means we can reflect this metric update in the recorded span data, for example.

jmacd · 2019-07-22T23:11:09Z

api/metric/api.go

+type Float64Measure interface {
+	Record(ctx context.Context, value float64, labels ...core.KeyValue)
+}
+
 type Handle struct {


A handle is a common base type for any of the metrics. In this proposal, the only API difference between the metric types is the verb they use, the interpretation is left to the SDK. So the Handle is a thing that describes the metric independently of its verb or metric type. The properties of a metric are: variable (name), type (gauge, additive, cumulative, or measure), keys (recommended), and aggregation (recommended).

jmacd · 2019-07-22T23:13:02Z

api/metric/api.go

-		return "unknown"
+// WithAggregation applies a user-recommended aggregation to this
+// metric, useful particularly for Measure type metrics.
+func WithAggregation(aggr aggregation.Operator) Option {


This is not a definition. This is a recommendation. The SDK is not bound to follow this advice. If this value is not set, the default aggregation for MeasureMetrics is None. I like this because the application programmer can suggest None as a good aggregation for measure metrics, but could also suggest Distribution.

jmacd · 2019-07-22T23:14:10Z

api/trace/current.go

@@ -25,10 +25,16 @@ var (
 )

 func SetCurrentSpan(ctx context.Context, span Span) context.Context {
+	if ctx == nil {


In my exporter, I want to know the current span in order to annotate the span data with accumulated metrics, grouped by span_id. I want this to happen for all callers, without the programmer having to do anything. So this check avoids a crash when you ask for the current span and there's no context.

jmacd · 2019-07-22T23:15:21Z

experimental/streaming/exporter/reader/reader.go

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


Here, this is where the metric update is associated with a current span. This would be a nil context if for some reason the programmer passed a nil to the metrics APIs.

* Add MinMaxSumCount stress test * Reimplement MinMaxSumCount using StateLocker * Address PR comments * Round #2 of PR comments Co-authored-by: Rahul Patel <rahulpa@google.com>

* Add ExportTimeout option * Adjust tests * Update CHANGELOG * Beef up the exporter timeout test * Beef up exporter test - attempt #2 Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

jmacd added 7 commits July 17, 2019 13:05

New metric types

528d9c8

Upstream

ecb6860

Implement the new metric types in experimental/streaming

d17e9ae

New file

1db27c8

Firm up the example

d88ee56

Reduce number of event types; use a stringer for MetricType

8ef1a27

Tidy

48032d7

jmacd mentioned this pull request Jul 20, 2019

Metrics API RFCs to eliminate Raw statistics open-telemetry/oteps#4

Merged

bogdandrutu reviewed Jul 22, 2019

View reviewed changes

jmacd commented Jul 22, 2019

View reviewed changes

Upstream

4df8dd1

jmacd closed this Aug 16, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics proposal for Go #2

Metrics proposal for Go #2

jmacd commented Jul 19, 2019 •

edited

Loading

bogdandrutu Jul 22, 2019

jmacd Jul 22, 2019

bogdandrutu Jul 22, 2019

bogdandrutu Jul 22, 2019

bogdandrutu Jul 22, 2019

bogdandrutu Jul 22, 2019

jmacd Jul 22, 2019

bogdandrutu Jul 22, 2019

jmacd Jul 22, 2019

bogdandrutu Jul 22, 2019

jmacd Jul 22, 2019

bogdandrutu Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

jmacd Jul 22, 2019

Metrics proposal for Go #2

Metrics proposal for Go #2

Conversation

jmacd commented Jul 19, 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

jmacd commented Jul 19, 2019 •

edited

Loading