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.

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

Allow for custom labels on prometheus metrics #393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matdurand
Copy link

Solves #392

}
}

func NewPrometheusMetricsBuilder(prometheusRegistry prometheus.Registerer, namespace string, subsystem string, opts ...Option) PrometheusMetricsBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@matdurand matdurand Sep 21, 2023

Choose a reason for hiding this comment

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

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
Copy link
Author

Choose a reason for hiding this comment

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

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.

@matdurand matdurand requested a review from m110 October 21, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants