feat: inject prometheus registry#3106
Conversation
github-actions
Bot
added
area/cli
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (70.09%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #3106 +/- ##
==========================================
- Coverage 75.52% 70.09% -5.42%
==========================================
Files 503 503
Lines 61820 62064 +244
==========================================
- Hits 46683 43499 -3184
- Misses 11722 15410 +3688
+ Partials 3415 3155 -260 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I've left a couple of comments. I generally like where this is going but there's a couple of things I'd want to touch up. I also want to get another couple of pairs of eyes on this - I'll poke some folks.
There's also a handful of tests failing. The unit tests are easy to run, but you can use the mage targets to make the e2e and integration tests easy to run. golangci-lint will also catch these kinds of things across the rest of the codebase.
| for _, c := range []prometheus.Collector{connectionsPerCRDBNodeCountGauge, pruningTimeHistogram} { | ||
| if err := registerer.Register(c); err != nil { | ||
| var alreadyRegistered prometheus.AlreadyRegisteredError | ||
| if !errors.As(err, &alreadyRegistered) { |
| if registerer == nil { | ||
| registerer = prometheus.DefaultRegisterer | ||
| } | ||
| for _, c := range []prometheus.Collector{connectionsPerCRDBNodeCountGauge, pruningTimeHistogram} { |
There was a problem hiding this comment.
It seems like this logic could be wrapped with a function like:
func util.RegisterPrometheusMetrics(registerer prometheus.Registerer, errMessage string, metrics... prometheus.Metric) error {}So that we can cut down on the duplication a bit.
Could also have it return the list of metrics for unregistration, or else have it return a closure that you can call in a Close function that does all of the unregistration for you.
Maybe package all that up in a struct? I'm thinking out loud here; take whatever parts of it make sense to you and work in the context.
There was a problem hiding this comment.
addressed in 8af3a73
RegisterPrometheusMetrics returning a closure.
| if cd.registerer != nil { | ||
| cd.registerer.Unregister(cd.checkTotalCounter) | ||
| cd.registerer.Unregister(cd.checkFromCacheCounter) | ||
| cd.registerer.Unregister(cd.lookupResourcesTotalCounter) | ||
| cd.registerer.Unregister(cd.lookupResourcesFromCacheCounter) | ||
| cd.registerer.Unregister(cd.lookupSubjectsFromCacheCounter) | ||
| cd.registerer.Unregister(cd.lookupSubjectsTotalCounter) | ||
| } | ||
|
|
There was a problem hiding this comment.
This could be cleaner if we have a struct that tracks these and has an Unregister method, or else the registration function returns a function for unregistering.
There was a problem hiding this comment.
We have need of the counters, ie to increment, so would be due to wrap all metrics interactions, not just register/unregister.
* Address linter, generated options. * Inject prometheus registerer.
* Add prometheus register utility, returning an unregister function. * Address failing tests.
2 participants
Description
Throughout, use of prometheus.Register and prometheus.Unregister lead to test isolation being compromised. This change makes the Prometheus Registerer an injectable.
As expected w/i #2829 , promauto use also needed to be removed.
I did not revert #2957 which worked around the test isolation issue.
Testing
[X] Execute existing unit tests.
[X] Manually test as follows:
References
#2829