add metric producer manager. #1039
Conversation
metric/producer/manager.go
Outdated
|
|
||
| type manager struct { | ||
| mu sync.RWMutex | ||
| producers []Producer |
There was a problem hiding this comment.
Consider using a map[string]Producermap[Producer]struct{}.
There was a problem hiding this comment.
i had considered that before but map cannot be unmodifiable. However slices are unmodifiable implicitly.
There was a problem hiding this comment.
You can implement your own immutable map/slice (also not sure that slices are immutable) by cloning the current map/slice and apply the mutation operation on the clone then swap the pointers to the map/slice.
metric/producer/manager.go
Outdated
| return pm.producers | ||
| } | ||
|
|
||
| func (pm *manager) contains(producer Producer) bool { |
There was a problem hiding this comment.
Suggestion: if contains is only used in add, maybe it's not worth adding a separate method; otherwise contains should be guarded with mu.
metric/producer/manager.go
Outdated
| // used by exporter to read metrics from producers. | ||
| func GetAll() []Producer{ | ||
| pm := getManager() | ||
| return pm.producers |
There was a problem hiding this comment.
Consider returning a copy of producer slice, otherwise this may introduce synchronization issue. In Java we always return an unmodifiable snapshot.
There was a problem hiding this comment.
slices are unmodifiable implicitly. I have added a test for it.
There was a problem hiding this comment.
That is not true:
I think this is a good article to read https://medium.com/@nitishmalhotra/uh-ohs-in-go-slice-of-pointers-c0a30669feee
There was a problem hiding this comment.
I gave you the wrong link. The idea is that Slice is a struct {len, cap, pointerToData}. If passed by value all of these will get copied but the underlying memory where pointerToData points is shared between multiple copies of the slice.
There was a problem hiding this comment.
I got it. It is NOT immutable. I verified by a different test.
There was a problem hiding this comment.
I will change the implementation.
|
@bogdandrutu and @songy23 PTAL again. |
| return prodMgr | ||
| } | ||
|
|
||
| // Add adds the producer to the manager if it is not already present. |
There was a problem hiding this comment.
Maybe define what is the "Manager"?
| func (pm *manager) getAll() []Producer { | ||
| pm.mu.Lock() | ||
| defer pm.mu.Unlock() | ||
| producers := make([]Producer, len(pm.producers)) |
There was a problem hiding this comment.
producers := make([]Producer, len(pm.producers))
copy(producers, pm.producers)
There was a problem hiding this comment.
producers is slice while pm.producers is a map. So copy won't work.
metric/metricexport/producer.go
Outdated
| @@ -19,6 +19,7 @@ import ( | |||
| ) | |||
|
|
|||
| // Producer is a source of metrics. | |||
| // Deprecated in lieu of go.opencensus.io.metric.producer.Producer | |||
There was a problem hiding this comment.
Simply delete it, we don't use this anywhere correct?
|
@rghetia ping me when this is ready for review. |
It is ready. |
| // Add adds the producer to the manager if it is not already present. | ||
| // The manager maintains the list of active producers. It provides | ||
| // this list to a reader to read metrics from each producer and then export. | ||
| func Add(producer Producer) { |
There was a problem hiding this comment.
I would like to have these function on a "ProducerManager" struct and have a "GetGlobalProducerManager". This way the exporters will get a "ProducerManager" as an option, this is a bit more flexible. We can do that in a separate PR just to make sure we make progress, but I think is a better design.
|
@odeke-em This is for the OpenCensus-Go main library to produce |
No description provided.