fix: Add struct-wide RWMutex to metrics #3421
Conversation
simster7
changed the title
| return | ||
| } | ||
| m.WorkflowDeleted(key, fromPhase) | ||
| m.WorkflowAdded(key, toPhase) | ||
| if _, ok := m.workflowsByPhase[fromPhase]; ok { |
There was a problem hiding this comment.
is this copy-and-paste? maybe make private func which does not lock?
There was a problem hiding this comment.
Not quite copy-and-paste, there is some logic that would be redundant if we go with the private func approach; mainly, we will remove and readd the key from the map and perform an additional check unnecessarily. I actually think this is simpler and easier to understand.
However, I don't feel strongly and would gladly go with the private func approach if you think it's necessary.
There was a problem hiding this comment.
we should just make sure that if we need to make changes in two places - we are aware of it
|
it's like in Java when you add |
| @@ -77,6 +77,9 @@ func New(metricsConfig, telemetryConfig ServerConfig) *Metrics { | |||
| } | |||
|
|
|||
| func (m *Metrics) allMetrics() []prometheus.Metric { | |||
| m.mutex.RLock() | |||
There was a problem hiding this comment.
will it handle concurrent read and write on map?
There was a problem hiding this comment.
there is a synchronized map in golang.
https://golang.org/pkg/sync/#Map
Checklist:
"fix(controller): Updates such and such. Fixes #1234".