Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: fix pd client metrics registration #8994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+30 −2
Open

client: fix pd client metrics registration #8994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions client/metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package metrics

import (
"sync"
"sync/atomic"

"github.com/prometheus/client_golang/prometheus"
Expand All @@ -26,6 +27,26 @@
func init() {
initMetrics(prometheus.Labels{})
initCmdDurations()
initRegisteredConsumers()
}

var mutex sync.Mutex
var consumersInitializers []func()
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using an atomic.Value to store []func()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just storing reference to []func() won't be enough as array append is not atomic operation in golang (it might trigger races in the resize flow). Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your consideration is thorough. My initial intention was to merge the lock on line 33 with line 34 into one structure, making it look more elegant.

Copy link
Contributor Author

@Tema Tema Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial intention was to merge the lock on line 33 with line 34 into one structure, making it look more elegant.

Do you still insist on the change? If so, could you please show me how you propose to make it work with atomic.Value which will be atomic and race free?


// RegisterConsumer registers a consumer to be initialized when the metrics are (re)initialized.
func RegisterConsumer(initConsumer func()) {
mutex.Lock()
defer mutex.Unlock()
consumersInitializers = append(consumersInitializers, initConsumer)
initConsumer()
}

func initRegisteredConsumers() {
mutex.Lock()
defer mutex.Unlock()
for _, initConsumer := range consumersInitializers {
initConsumer()
}

Check warning on line 49 in client/metrics/metrics.go

View check run for this annotation

Codecov / codecov/patch

client/metrics/metrics.go#L48-L49 

Added lines #L48 - L49 were not covered by tests
}

// InitAndRegisterMetrics initializes and registers the metrics manually.
Expand All @@ -34,6 +55,7 @@
// init metrics with constLabels
initMetrics(constLabels)
initCmdDurations()
initRegisteredConsumers()
// register metrics
registerMetrics()
}
Expand Down
10 changes: 8 additions & 2 deletions client/pkg/circuitbreaker/circuit_breaker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,18 @@ func NewCircuitBreaker(name string, st Settings) *CircuitBreaker {
cb.config = &st
cb.state = cb.newState(time.Now(), StateClosed)

metricName := replacer.Replace(name)
m.RegisterConsumer(func() {
registerMetrics(cb)
})
return cb
}

func registerMetrics(cb *CircuitBreaker) {
metricName := replacer.Replace(cb.name)
cb.successCounter = m.CircuitBreakerCounters.WithLabelValues(metricName, "success")
cb.errorCounter = m.CircuitBreakerCounters.WithLabelValues(metricName, "error")
cb.overloadCounter = m.CircuitBreakerCounters.WithLabelValues(metricName, "overload")
cb.fastFailCounter = m.CircuitBreakerCounters.WithLabelValues(metricName, "fast_fail")
return cb
}

// ChangeSettings changes the CircuitBreaker settings.
Expand Down
Loading