Add metrics Enricher API #1271

hstan · 2020-10-21T10:37:47Z

This PR implements a way to correlate context(e.g. span context) with metrics by introducing a MetricsProcessor interface which can be implemented by the client and provided when creating a new push controller.

Usecase:

Assuming we have a requirement to associate our spans with metrics by storing the span id as metrics label, first we create an implementation of the MetricsProcessor and provided it during the creation of the push controller.

type BaggageMetricsLabelsEnricher struct {} // Implements MetricsProcessors

func (p *BaggageMetricsLabelsEnricher) OnMetricRecorded(ctx context.Context, kvs *[]label.KeyValue) {
	baggage := otel.Baggage(ctx)
	*kvs = append(baggage.ToSlice(), *kvs...)
}

pusher := push.New(
	processor.New(selector, metric.PassThroughExporter),
	exporter,
	push.WithMetricsProcessor(new(BaggageMetricsLabelsEnricher)),
)

Then when recording a metric, we can provide a context which includes span information as well as other cross-cutting metadata

spanContext, span := tracer.Start(ctx, name)
...
ctx = MyContextWithSpanContext(spanContext, span.Context())
recorder, _ := meter.NewFloat64ValueRecorder("my.recorder")
recorder.Record(ctx, val)

As the processor gets applied for all recorded metrics, no additional explicit label setting is required when recording a value.

Related discussion: open-telemetry/opentelemetry-specification#1116

codecov · 2020-10-21T10:40:48Z

Codecov Report

Merging #1271 (6dab82e) into master (af114ba) will increase coverage by 0.0%.
The diff coverage is 85.0%.

@@          Coverage Diff           @@
##           master   #1271   +/-   ##
======================================
  Coverage    78.0%   78.1%           
======================================
  Files         123     124    +1     
  Lines        6252    6269   +17     
======================================
+ Hits         4879    4897   +18     
  Misses       1125    1125           
+ Partials      248     247    -1

Impacted Files	Coverage Δ
sdk/metric/sdk.go	`79.5% <75.0%> (-0.7%)`	⬇️
sdk/metric/config.go	`100.0% <100.0%> (ø)`
sdk/metric/controller/pull/config.go	`66.6% <100.0%> (+16.6%)`	⬆️
sdk/metric/controller/pull/pull.go	`100.0% <100.0%> (ø)`
sdk/metric/controller/push/config.go	`75.0% <100.0%> (+8.3%)`	⬆️
sdk/metric/controller/push/push.go	`96.3% <100.0%> (+<0.1%)`	⬆️
sdk/resource/resource.go	`67.5% <0.0%> (+10.0%)`	⬆️

chenzhihao · 2020-10-22T04:57:06Z

sdk/metric/sdk.go

+		// OnMetricRecorded is execute everytime a metric is recorded by
+		// the sync instrument implementation of an Accumulator, it generally
+		// provides ability to correlate the context with the metrics
+		OnMetricRecorded(context.Context, *[]label.KeyValue)


would this method be too specific?

…nto add-metrics-processor

CHANGELOG.md

sdk/metric/config.go

sdk/metric/controller/push/config.go

sdk/metric/sdk.go

Aneurysm9 · 2020-10-28T22:12:57Z

sdk/metric/sdk.go

@@ -291,6 +308,9 @@ func (s *syncInstrument) Bind(kvs []label.KeyValue) api.BoundSyncImpl {
 }

 func (s *syncInstrument) RecordOne(ctx context.Context, number api.Number, kvs []label.KeyValue) {
+	for _, processor := range s.getMetricsProcessors() {
+		processor.OnMetricRecorded(ctx, &kvs)


It seems that the processors are only invoked for synchronous instruments, do they not have any value for async instruments?

@Aneurysm9 TBH we haven't started using the async instrument yet, but when implementing this feature I did looked it. Firstly I didn't find a proper place to add this hook then when I explore the code I reckon it is unnecessary to do that since for async instrument the client can provide a callback which already gives ability to achive the same purpose.
e.g. https://github.com/open-telemetry/opentelemetry-go/blob/master/example/basic/main.go#L71 you can re-compose the metric labels using the context and commonLabels in the callback function. (Please correct me if I'm wrong)

It makes sense to use this interface for asynchronous instruments, but the applications will be different. There's no distributed context available for this Enricher API, but you still might want to use this API to filter labels before they enter the Accumulator. Enricher is not a good name for a filter, so it feels like we still haven't found the right name for this concept.

@jmacd Have you had a chance to look open-telemetry/opentelemetry-specification#1116 (comment)

hstan · 2020-10-30T01:31:47Z

@Aneurysm9 Thanks for reviewing. I've addressed your comment, could you please take another look?

hstan · 2020-11-02T04:23:30Z

@Aneurysm9 I was checking the conflicts and found this PR #1235 has removed the config and changed the structure of the code. Could you suggest how should I structure my code according to this change?

cc @jmacd since you made the change and we're in another conversation at open-telemetry/opentelemetry-specification#1116 Do you think I should add the config option back for the "AccumulatorProcessor" or pass it as another parameter in addition to the resource for NewAccumulator ?

MrAlias

I'm hesitant to add this interface without a broader discussion at the specification level. In the past we have prototyped concepts here that were ultimately destine to become additions to the specification. Is that the intent of this PR @hstan?

hstan · 2020-11-10T04:13:38Z

I'm hesitant to add this interface without a broader discussion at the specification level. In the past we have prototyped concepts here that were ultimately destine to become additions to the specification. Is that the intent of this PR @hstan?

As I mentioned in the description, this is part of the discussion on the specification linked below.
open-telemetry/opentelemetry-specification#1116

This was intended to demo a go implementation to support the discussion but also we want it to be merged at some stage since we need this feature. Besides the discussion above @parallelstream also had a chat with @bogdandrutu on this and at high level.

MrAlias · 2020-11-10T22:07:38Z

sdk/metric/sdk.go

+
+	// Implementations of MetricsProcessor can be provided as an config option to provide an opportunity
+	// to re-compose metrics labels based on the context when the metrics are recorded.
+	MetricsProcessor interface {


MetricsProcessor does not seem like an appropriate name for this interface. It is overly general, uses the word "metrics" as a noun instead of an adjective (something that is avoided in OpenTelemetry), and duplicates the name of the other metric.Processor from the export package, which is is also a field of the Accumulator.

Can we use a more descriptive name of the function this interface will serve and possibly match broader naming standards?

MrAlias · 2020-11-10T22:15:47Z

sdk/metric/sdk.go

+		// OnMetricRecorded is execute everytime a metric is recorded by
+		// the sync instrument implementation of an Accumulator, it generally
+		// provides ability to correlate the context with the metrics
+		OnMetricRecorded(context.Context, *[]label.KeyValue)


I'm guessing the added indirection here of a reference type for the second parameter means that argument will be mutated. This doesn't seem idiomatic of Go and it will likely add an additional allocation of the slice pointer and pose concurrency issues.

Is there any reason this could not accept a slice and return a slice?

Additionally, how are errors expected to be handled here?

sdk/metric/controller/push/push.go

MrAlias · 2020-11-10T22:26:24Z

sdk/metric/sdk.go

+
+	// Implementations of MetricsProcessor can be provided as an config option to provide an opportunity
+	// to re-compose metrics labels based on the context when the metrics are recorded.
+	MetricsProcessor interface {


Does this need to be an interface? It seems like a declared type of func would be sufficient here.

I think you're right

…etrics-processor

…ingful name

hstan · 2020-11-11T12:01:27Z

sdk/metric/sdk.go

+
+	// MetricsLabelsEnricher can be provided as a config option to enrich metrics labels based on
+	// the context when the metrics are recorded
+	MetricsLabelsEnricher func(context.Context, []label.KeyValue) ([]label.KeyValue, error)


@MrAlias Does this make sense?

jmacd · 2020-11-12T00:58:48Z

Sorry for the delay. I'll try to review it before tomorrow's SIG meetings.

jmacd

This is great. My only reservation is still about terminology. In recent Metrics SIGs we've briefly discussed using the name "Processor" for the interface here, since it's closer to what the Span Processor API does. That leaves us hunting for a new name for the existing metrics Processor component in the SDK specification. Earlier names for that Processor: "Batcher", and "Integrator". We should discuss this in the SIG tomorrow.

jmacd · 2020-11-12T07:28:05Z

sdk/metric/config.go

+package metric
+
+// Config contains configuration for an SDK.
+type Config struct {


FWIW I recently removed a similar struct from this package, arguing that Accumulators are not generally built from options, but from one of the controllers. That's why Resource is an explicit argument, and I would be just as happy if this were an third argument to the function, as opposed to a new option. This would be the first optional setting, and maybe we can introduce a Config struct an Option interface when there are more than one.

FWIW, I'm planning to consolidate the push and pull controllers, which would simplify this PR. See open-telemetry/opentelemetry-specification#1207.

jmacd · 2020-11-12T07:28:55Z

sdk/metric/controller/push/config.go

+
+	// MetricsLabelsEnricher is a function that enriches metrics labels based
+	// on kvs stored in context when metrics are recorded.
+	MetricsLabelsEnricher metric.MetricsLabelsEnricher


(Repeat: open-telemetry/opentelemetry-specification#1207 says we can and should consolidate push and pull controllers, to avoid this repetition.)

sdk/metric/sdk.go

jmacd · 2020-11-12T07:38:00Z

sdk/metric/sdk.go

@@ -305,11 +321,17 @@ func (s *syncInstrument) RecordOne(ctx context.Context, number api.Number, kvs [
 // processor will call Collect() when it receives a request to scrape
 // current metric values.  A push-based processor should configure its
 // own periodic collection.
-func NewAccumulator(processor export.Processor, resource *resource.Resource) *Accumulator {
+func NewAccumulator(processor export.Processor, resource *resource.Resource, opts ...Option) *Accumulator {


See the comment above. I'd be happier to leave this a three-argument function until we have more than one optional setting.

…nto add-metrics-processor

jmacd · 2020-12-11T08:12:57Z

sdk/metric/sdk.go

@@ -291,6 +298,15 @@ func (s *syncInstrument) Bind(kvs []label.KeyValue) api.BoundSyncImpl {
 }

 func (s *syncInstrument) RecordOne(ctx context.Context, number api.Number, kvs []label.KeyValue) {
+	if s.meter.metricsLabelsEnricher != nil {
+		var err error
+		kvs, err = s.meter.metricsLabelsEnricher(ctx, kvs)


This looks good for the unbound instrument code path. 👍

For the bound code path, in:

func (r *record) RecordOne(ctx context.Context, num number.Number)

If the enricher is present, take a new code path, where you'll call

s.meter.metricsLabelsEnricher(ctx, record.labels.ToSlice())

to get the enriched labels and then divert to the unbound instrument code path, since the *record that was bound is not applicable after enrichment of its labels.

jmacd · 2020-12-11T17:58:36Z

@bogdandrutu and @open-telemetry/specs-metrics-approvers do you think this is an API-level or an SDK-level concept?

jkwatson · 2020-12-11T19:13:12Z

@bogdandrutu and @open-telemetry/specs-metrics-approvers do you think this is an API-level or an SDK-level concept?

this sounds like SDK to me.

…nto add-metrics-processor # Conflicts: # CHANGELOG.md

hstan · 2020-12-18T04:02:00Z

I will continue this PR after #1378 gets merged

evantorrie · 2021-01-15T22:53:35Z

@hstan #1378 has been merged. I'm interested in this API as well, so interested to see if you've returned to this.

I will continue this PR after #1378 gets merged

hstan · 2021-01-19T02:41:41Z

@jmacd Are you working on adding the setting to the controller as you mentioned in #1421 ? If not I can have a go.

hstan · 2021-01-20T01:14:38Z

closing this PR and starting a new branch to implement this based on #1421

hstan · 2021-01-21T03:40:29Z

opened new PR #1480

hstan added 4 commits October 21, 2020 02:22

add metrics processor

468688c

remove unused method

fc0b575

move interface and add comments

4e641ac

add comments for MetricsProcessor interface

fe2c130

hstan requested review from Aneurysm9, evantorrie, jmacd, lizthegrey, MrAlias and XSAM as code owners October 21, 2020 10:37

hstan mentioned this pull request Oct 21, 2020

Document Metrics Processor APIs and SDK requirements open-telemetry/opentelemetry-specification#1116

Closed

Merge branch 'master' into add-metrics-processor

dbe97fe

chenzhihao reviewed Oct 22, 2020

View reviewed changes

hstan changed the title ~~[WIP] Add metrics processor~~ Add metrics processor Oct 28, 2020

hstan added 2 commits October 28, 2020 13:47

add changelog

b297e94

Merge branch 'master' of github.com:open-telemetry/opentelemetry-go i…

ee0f8fd

…nto add-metrics-processor

Aneurysm9 reviewed Oct 28, 2020

View reviewed changes

hstan added 3 commits October 29, 2020 23:19

make the option api accept variadic signature for metrics processors

21c833f

split changelog into two separate lines

7b11574

update MetricsProcessor documentation

015599a

hstan requested a review from Aneurysm9 October 30, 2020 02:28

MrAlias reviewed Nov 9, 2020

View reviewed changes

MrAlias added area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package prototype Feature to prototype a spec-level decision labels Nov 10, 2020

MrAlias reviewed Nov 10, 2020

View reviewed changes

hstan added 8 commits November 11, 2020 20:11

Merge branch 'master' of github.com:hstan/opentelemetry-go into add-m…

adf34f1

…etrics-processor

use a function type instead of interface and rename it to a more mean…

e6b051e

…ingful name

update changelog

afa378d

add metrics labels enricher to pull controller

8ea1616

add test for pull with metrics labels enricher

f73d433

add test for push with metrics labels enricher

208a049

sort import order

4f91f17

update changelog

4739df6

hstan commented Nov 11, 2020

View reviewed changes

handle error returned from the enricher function

85faa2e

jmacd reviewed Nov 12, 2020

View reviewed changes

Merge branch 'master' of github.com:open-telemetry/opentelemetry-go i…

b94b328

…nto add-metrics-processor

jmacd changed the title ~~Add metrics processor~~ Add metrics Enricher API Dec 3, 2020

jmacd reviewed Dec 11, 2020

View reviewed changes

hstan added 3 commits December 14, 2020 13:58

clean up

b3caafa

Merge branch 'master' of github.com:open-telemetry/opentelemetry-go i…

a22e258

…nto add-metrics-processor # Conflicts: # CHANGELOG.md

fix tests

6dab82e

jmacd mentioned this pull request Dec 23, 2020

Metric SDK label Enricher interface #1421

Closed

hstan closed this Jan 20, 2021

hstan mentioned this pull request Jan 21, 2021

Metric label enrich #1480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics Enricher API #1271

Add metrics Enricher API #1271

hstan commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

chenzhihao Oct 22, 2020 •

edited

Loading

Aneurysm9 Oct 28, 2020

hstan Oct 29, 2020

jmacd Dec 4, 2020

hstan Dec 4, 2020

hstan commented Oct 30, 2020

hstan commented Nov 2, 2020 •

edited

Loading

MrAlias left a comment

hstan commented Nov 10, 2020

MrAlias Nov 10, 2020

MrAlias Nov 10, 2020

MrAlias Nov 10, 2020

hstan Nov 11, 2020

hstan Nov 11, 2020

jmacd commented Nov 12, 2020

jmacd left a comment

jmacd Nov 12, 2020

jmacd Nov 12, 2020

jmacd Nov 12, 2020

jmacd Dec 11, 2020

jmacd commented Dec 11, 2020

jkwatson commented Dec 11, 2020

hstan commented Dec 18, 2020

evantorrie commented Jan 15, 2021

hstan commented Jan 19, 2021

hstan commented Jan 20, 2021

hstan commented Jan 21, 2021

Add metrics Enricher API #1271

Add metrics Enricher API #1271

Conversation

hstan commented Oct 21, 2020 • edited Loading

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

chenzhihao Oct 22, 2020 • 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

hstan commented Oct 30, 2020

hstan commented Nov 2, 2020 • edited Loading

MrAlias left a comment

Choose a reason for hiding this comment

hstan commented Nov 10, 2020

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 Nov 12, 2020

jmacd 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

jmacd commented Dec 11, 2020

jkwatson commented Dec 11, 2020

hstan commented Dec 18, 2020

evantorrie commented Jan 15, 2021

hstan commented Jan 19, 2021

hstan commented Jan 20, 2021

hstan commented Jan 21, 2021

hstan commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

chenzhihao Oct 22, 2020 •

edited

Loading

hstan commented Nov 2, 2020 •

edited

Loading