[WIP] Fix incorrect metrics getting generated from multiple readers #5900
Conversation
…w, once the fix goes it will pass
pree-dew
requested review from
MrAlias,
XSAM,
dashpole,
pellared and
dmathieu
as code owners
dashpole
added
bug
…ipeline and get the measure during collect for the called pipeline
| switch o := inst.(type) { | ||
| case int64Observable: | ||
| if e := o.registerable(m); e != nil { | ||
| if !errors.Is(e, errEmptyAgg) { |
There was a problem hiding this comment.
I think this will return the same error multiple times (once for each pipeline). I think we should iterate over instruments once to see if it is registerable, and construct a list of valid instruments. Then iterate over valid instruments once for each pipe and register the valid ones.
There was a problem hiding this comment.
makes sense. I will make the change
| for i, pipe := range p { | ||
| unregs[i] = pipe.addMultiCallback(c) | ||
| } | ||
| func (p pipelines) registerMultiCallbacks(unregs []func()) metric.Registration { |
There was a problem hiding this comment.
nit: consider removing this function, and just returning unregisterFuncs{f: unregs} from RegisterCallback
| } | ||
|
|
||
| type observer struct { | ||
| embedded.Observer | ||
|
|
||
| pipe *pipeline | ||
| float64 map[observableID[float64]]struct{} |
There was a problem hiding this comment.
If you take my advice above to do validation once for all pipelines, you should be able to remove these maps from the observer. IIRC, they are only used for validation, so you could just create these maps at the beginning of RegisterCallback.
| @@ -530,7 +546,10 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... | |||
| return | |||
| } | |||
| c := metric.NewObserveConfig(opts) | |||
| oImpl.observe(v, c.Attributes()) | |||
| measures := r.pipe.float64Measures[oImpl.observableID] | |||
There was a problem hiding this comment.
Do we need to lock around this? It would be good to ensure our parallel tests Invoke a callback in parallel with RegisterCallbacks
There was a problem hiding this comment.
Sure, will make the change
Will fix #5866