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

Prometheus: NewPrometheus constructor shall accept a config struct #92

Closed
wants to merge 2 commits into from

Conversation

yabberyabber
Copy link

@yabberyabber yabberyabber commented Mar 23, 2023

The prometheus middleware is difficult to extend. Not only because the existing NewPrometheus constructor has no way to extend the arguments passed, but also the imperative Prometheus.SetFoo style of configuration that is currently supported is difficult to reason about.

This PR creates a new constructor called prometheus.NewPrometheusWithConfig which accepts a prometheus.PrometheusConfig struct as an argument. The PrometheusConfig struct is by nature much more extensible because additional fields can be invented without breaking any existing clients. This resolves the issue referenced in #80

This PR uses this new PrometheusConfig struct to add 2 forms of extensibility:

  • The PrometheusConfig.LabelDescriminators field can be used to add additional labels to the metrics in prometheus. The keys of this mapping are the names of the labels to add. The values in this mapping are the discriminators that will be used to populate these fields. This resolves the issue referenced in Is there a way to add custom labels to the standard prometheus metrics? #85
    • Special considerations were made to maintain backwards-compatability with the existing RequestCounterURLLabelMappingFunc and RequestCounterHostLabelMappingFunc attributes. Previously, users would override these attributes after constructing the prometheus instance. This is a little awkward to support, but I've managed to do so in this PR by maintaining their default values and referencing their closure from the Prometheus.labelDescriminators["code"] entry.
    • Users can still override the "code" or "entry" label mapping functions by setting the associated Prometheus members, but I would suggest they instead look at defining those as label desciminators which I believe exposes a more intuitive interface
  • The PrometheusConfig.Registerer and PrometheusConfig.Gatherer allow users to use a prometheus registry other than prometheus.DefaultRegistery - this is particularly helpful in unit tests. This resolves the issue referenced in Mock Prometheus Middleware in unit tests echo#2419 . I initially hoped to handle this registry issue in a separate PR, but I needed the ability to specify my own registry in order to properly test the LabelDescriminators code because prometheus would otherwise complain that the labels are inconsistent between test cases.

Testing done

Please note that there are many test cases referencing the old prometheus.NewPrometheus constructor. This constructor now uses the new prometheus.NewPrometheusWithConfig constructor under the hood, so those existing unit tests provide a reasonable degree of confidence that there haven't been any regressions.

I've added an additional test case to cover the LabelDescriminators config piece. This test case defines an additional label called userAgent along with a function that populates this label given the User-Agent header from the request. This new test also covers the PrometheusConfig.{Registerer,Gatherer} fields. If we were to use the prometheus.DefaultRegistry here, the middleware would get an error when trying to register the labels because the labels from this test are different from the labels from other tests. Using a private registry prevents the conflict.

This change does not bring any additional functionality, but it paves
the way to provide extra pieces of configuration in the future.
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 85.57% and project coverage change: +4.35 🎉

Comparison is base (b6855c2) 58.84% compared to head (957252e) 63.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   58.84%   63.19%   +4.35%     
==========================================
  Files           8        8              
  Lines         690      758      +68     
==========================================
+ Hits          406      479      +73     
+ Misses        260      256       -4     
+ Partials       24       23       -1     
Impacted Files Coverage Δ
prometheus/prometheus.go 62.17% <85.57%> (+13.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yabberyabber yabberyabber marked this pull request as ready for review March 23, 2023 19:57
@aldas
Copy link
Contributor

aldas commented Mar 27, 2023

@yabberyabber what do you think of #94 ?

@aldas
Copy link
Contributor

aldas commented May 22, 2023

closing - we have now #94

@aldas aldas closed this May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants