Use Promauto.With() where possible #3263
Conversation
roidelapluie
force-pushed
the
promauto
branch
2 times, most recently
from
d462aff
to
7c89441
Compare
| @@ -47,8 +47,8 @@ type alertmanagerMetrics struct { | |||
| silencesPropagatedMessagesTotal *prometheus.Desc | |||
| } | |||
|
|
|||
| func newAlertmanagerMetrics() *alertmanagerMetrics { | |||
| return &alertmanagerMetrics{ | |||
| func newAlertmanagerMetrics(r prometheus.Registerer) *alertmanagerMetrics { | |||
There was a problem hiding this comment.
alertmanagerMetrics implements prometheus.Collector interface. Collectors typically don't register into registry themselves, but are registered by client code using them. I think we should keep that, and not pass registerer into newAlertmanagerMetrics.
pkg/util/metrics_helper.go
Outdated
| @@ -465,11 +465,15 @@ func (d *HistogramData) Metric(desc *prometheus.Desc, labelValues ...string) pro | |||
| } | |||
|
|
|||
| // Creates new histogram data collector. | |||
| func NewHistogramDataCollector(desc *prometheus.Desc) *HistogramDataCollector { | |||
| return &HistogramDataCollector{ | |||
| func NewHistogramDataCollector(r prometheus.Registerer, desc *prometheus.Desc) *HistogramDataCollector { | |||
There was a problem hiding this comment.
As commented earleir, collectors should not require registrerer.
|
In general, I don't think object should register itself anywhere in its "new" method. (I'm sure you will find many counter-examples in Cortex codebase). |
pkg/ruler/manager.go
Outdated
| if reg != nil { | ||
| reg.MustRegister(userManagerMetrics) | ||
| } | ||
| userManagerMetrics := NewManagerMetrics(reg) |
There was a problem hiding this comment.
You can now initialise it directly below:
userManagerMetrics: NewManagerMetrics(reg),
```
There was a problem hiding this comment.
Do we want this New(reg) pattern or not? @pstibrany says no
There was a problem hiding this comment.
Sorry @roidelapluie for the long reply. Me and Peter had a discussion offline. We have a different opinion here. I commit to Peter's take: given ManagerMetrics is a Collector, and a Collector shouldn't take the registerer, let's not pass the registerer to NewManagerMetrics().
Same applies to alertmanager metrics.
roidelapluie
force-pushed
the
promauto
branch
from
7c89441
to
85b04bd
Compare
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
roidelapluie
force-pushed
the
promauto
branch
from
85b04bd
to
57b9f20
Compare
Also ensures that other standalone objects are registered by adding a
registerer to their New() function.
Signed-off-by: Julien Pivotto roidelapluie@inuits.eu
What this PR does:
Which issue(s) this PR fixes:
Partially #2204 (still miss the default registry metrics)
Checklist
Tests updatedthere are existing tests covering thisDocumentation addedCHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]