Allow for custom labels on prometheus metrics #393
Conversation
components/metrics/builder.go
Outdated
| } | ||
| } | ||
|
|
||
| func NewPrometheusMetricsBuilder(prometheusRegistry prometheus.Registerer, namespace string, subsystem string, opts ...Option) PrometheusMetricsBuilder { |
There was a problem hiding this comment.
Hey @matdurand, sorry about the delay. I wanted to comment last week and just realized I didn't do it. 🤦♂️
I have just one comment regarding the API — most Watermill components use a "Config" struct instead of the Option pattern, and I feel it would be a good idea to be consistent, just for the improved developer experience. This means we would need another constructor, something like NewPrometheusMetricsBuilderWithConfig. What do you think?
There was a problem hiding this comment.
Hey @m110, yeah sure, consistency is paramount. I replaced Option with config. I also renamed "custom" for "additional", just because it sounded better imo. Let me know if these changes are enough.
I also fixed an issue do deduplicate if you pass an additionalLabel that is already in the base labels. I had that issue locally in my test project because I needed to override the "publisher_name" label for a special case.
| @@ -46,3 +46,30 @@ func labelsFromCtx(ctx context.Context, labels ...string) prometheus.Labels { | |||
|
|
|||
| return ctxLabels | |||
| } | |||
|
|
|||
| type LabelComputeFn func(msgCtx context.Context) string | |||
There was a problem hiding this comment.
We might want to reflect on that signature. Is it enough to use only the context to get a label's value? Should we also pass the message pointer? For my use case, the additional labels are static, so I basically do this
metrics.MetricLabel{
Label: "service",
ComputeFn: func(ctx context.Context) string {
return "my_service"
},
},
but there might be a use case for getting something from the message itself as a label value.
Solves #392