Metric SDK label Enricher interface#1421
Metric SDK label Enricher interface#1421jmacd wants to merge 9 commits intoopen-telemetry:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1421 +/- ##
======================================
Coverage 78.6% 78.7%
======================================
Files 126 126
Lines 6367 6384 +17
======================================
+ Hits 5009 5025 +16
- Misses 1114 1115 +1
Partials 244 244
|
punya
left a comment
punya
left a comment
There was a problem hiding this comment.
A few small localized comments, I am still learning about the overall codebase.
| // 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, enricher export.Enricher) *Accumulator { |
There was a problem hiding this comment.
Is it worth using a functional option varargs parameter instead of adding enricher as a required parameter? It would make the call sites more readable, and preserve compatibility for existing calls (in particular, this diff would be shorter).
| // Return the enricher result if it is non-nil and no error. | ||
| if err == nil && out != nil { | ||
| return out | ||
| } | ||
| otel.Handle(err) |
There was a problem hiding this comment.
If out == nil, we will invoke otel.Handle(nil). I couldn't find any documentation for what otel.Handle is supposed to do in that case.
| // Return the enricher result if it is non-nil and no error. | |
| if err == nil && out != nil { | |
| return out | |
| } | |
| otel.Handle(err) | |
| if err != nil { | |
| otel.Handle(err) | |
| return kvs | |
| } | |
| if out == nil { | |
| return kvs | |
| } | |
| return out |
|
@jmacd my apologies, please feel free to disregard my review comments if you wish - I somehow failed to notice that this is a draft. |
|
@bogdandrutu Please review. |
|
I will re-open this later if needed, closing this as we have #1271. |
2 participants
This adds an
EnricherAPI that supports attaching baggage attributes as metric labels.This was a core feature of OpenCensus that OTel metrics hasn't implemented here, and we think this support will be specified for the default SDK.
Inspired by #1271 (@hstan) and my comments there.
This PR is a draft until #1378 merges, after which I'll add a setting to the controller and an example.