refactor ReadAll to Read and fix concurrency issue. #1056
Conversation
metric/registry.go
Outdated
| ) | ||
|
|
||
| // Registry creates and manages a set of gauges. | ||
| // External synchronization is required if you want to add gauges to the same | ||
| // registry from multiple goroutines. | ||
| type Registry struct { | ||
| mu sync.RWMutex | ||
| gauges map[string]*gauge |
There was a problem hiding this comment.
Maybe consider to use sync.Map? Please let me know if that matches your requirements.
There was a problem hiding this comment.
sync.Map would work.
| if ok { | ||
| existing := val.(*gauge) | ||
| if existing.isFloat != g.isFloat { |
There was a problem hiding this comment.
As we discussed we need to return an error if it exists not only if it has a different type. But happy to see that in a different PR.
There was a problem hiding this comment.
I will create a different PR for this.
| ms := make([]*metricdata.Metric, 0, len(r.gauges)) | ||
| for _, g := range r.gauges { | ||
| // Read reads all gauges in this registry and returns their values as metrics. | ||
| func (r *Registry) Read() []*metricdata.Metric { |
There was a problem hiding this comment.
Why change this name? I don't feel strongly about it but was curios.
There was a problem hiding this comment.
Producer interface is Read(). Since there is only going to be one Read there is no point in naming ReadAll. (short is better as long as it is clear).
If ReadAll is preferable I can change the producer interface.
No description provided.