Rework Prometheus middleware configuration

[aldas]

instead of NewPrometheus(subsystem string, skipper middleware.Skipper, customMetricsList ...[]*Metric) that has configuration as list of arguments we should have single argument that is configuration struct. This way it is easier to add new options.

[aldas]

Prometheus is already that "struct" but with those methods and private fields it also much more. starting servers and stuff like that. I think there should be clearer separation of configuring Prometheus and running server goroutines / handlers.

[yabberyabber]

Hey Aldas - I'm running into #85 and was looking at how I might implement it in a PR, but I ran into this problem. There doesn't currently appear to be any way to pass in config when constructing a prometheus middleware instance.

I'd like to propose a new API which I can implement/submit once I get a confirmation from a maintainer. The new API would look like this:

Phase 1 - provide extensible API to plumb config

type NewPrometheusOpts struct {
   CustomMetricsList []*Metric
   Skipper middleware.Skipper
}

func NewPrometheusWithOptions(subsystem string, opts NewPrometheusOpts) *Prometheus {
    return NewPrometheus(subsystem, opts.skipper, opts.customMetricsList)
}

Since it's a new API it won't break backwards compat. This fooWithOptions pattern is at least somewhat common with examples being seen in the go-git project when they had a similar need to add an options struct while maintaining backwards compatibility.

Phase 2 - add custom labels config

Since the thing I really want to support is the issue described in #85, I would add a new field to the NewPrometheusOpts struct called something like LabelDescriminators map[string]func(e echo.Context)string. The usage might look like the following:

p := NewPrometheusWithOptions("my_server", NewPrometheusOpts{
   // LabelDescriminators provides a way to optionally specify additional labels to add to
   // prometheus metrics. The label name comes from the key and the value is the result
   // of running the provided function on the request context.
   LabelDescriminators: map[string]func(e echo.Context)string{
      // the presence of a key called "client" triggers the constructor to create a "client"
      // label on all metrics. The value of this "client" label will be set by running the function
      // specified in this map.
      "client-type": func(e echo.Context) string {
          if hdr := e.Request().Header.Get("X-Client-type"); hdr != "" {
            return hdr
          }
          return "(unspecified)"
      },
   },
})

Please let me know if this interface matches the roadmap for the prometheus middleware. Like I said I'm happy to implement this and submit a PR but I would like to get some sort of confirmation before I go too far down this path.

[aldas]

usual Echo middleware conventions could be seen here https://github.com/labstack/echo/blob/f22ba6725c66896efdef029aaeb3bfc471f171c3/middleware/basic_auth.go#L14

type BasicAuthConfig struct {
//...
}

and mw is created from configs as:

func BasicAuthWithConfig(config BasicAuthConfig) echo.MiddlewareFunc {
//...
}

also I strongly suggest making list of old issues/PRs related to Prometheus MW and think of what people seem to be needing to configure.

For example couple days ago there was this https://github.com/labstack/echo/discussions/2419 I have not investigated this much but this global registering thing seems wrong. I assume there are ways to avoid relying on global vars etc.

This is because middleware.NewPrometheus() registers metrics into global register here https://github.com/labstack/echo-contrib/blob/b6855c2b7d9745b7db2498623e89cae526b41f57/prometheus/prometheus.go#L381

[yabberyabber]

It sounds like we are generally in agreement then. Here's an updated proposal that I think addresses both issues:

type PrometheusConfig struct {
    LabelDescriminators map[string]func(echo.Context)string

    Registerer prometheus.Registry
}
func NewPrometheusWithConfig(namespace string, config PrometheusConfig) *Prometheus {
}

Then this snippet right here https://github.com/labstack/echo-contrib/blob/b6855c2b7d9745b7db2498623e89cae526b41f57/prometheus/prometheus.go#L381 could access the provided Registerer (or default to prometheus.DefaultRegisterer if none is provided).

[aldas]

seems ok.

another thing https://github.com/labstack/echo-contrib/blob/b6855c2b7d9745b7db2498623e89cae526b41f57/prometheus/prometheus.go#L399 This method does too much - registering middleware and setting handler should be 2 distinct actions. If I recall correctly Prometheus could PUSH metrics also - therefore assumption that handler is always needed is not correct.

I think we should not have that much "automagick" and hidden things. ala go p.router.Start(p.listenAddress) is hidden in SetMetricsPath method

[yabberyabber]

I was just starting to notice this same thing as I was implementing the changes on my side :) I think that for backwards-compatability, prometheus.NewPrometeus will need to do the "automagick" thing and register it. But I can change that contract in prometheus.NewPrometheusWithConfig so that we only register it under certain circumstances.

How would you prefer this to work in the prometheus.NewPrometheusWithConfig scheme? Would you prefer if we made p.PrometheusHandler() public (it's currently private) and relied on the user to do something like this?

p := prometheus.NewPrometheusWithConfig(subsystem, &PrometheusConfig{})
p.Use(e)
e.GET("/metrics", p.PrometheusHandler())

[yabberyabber]

I've just posted a draft PR which handles everything except for the expansive prometheus.Use implementation. Let me know what you think

[aldas]

I'll look this evening but I was actually thinking of creating new middleware and mark old one deprecated. I that way whoever implements that new mw can start from clean sheet.

[yabberyabber]

Maintaining backwards compatibility was a little bit awkward but definitely not impossible. I think the PR as written honors the current behavior while providing a straightforward path to extension. The only thing I wasn't able to address is the overexpansive prometheus.Use which I don't think is a huge issue and which could be solved with the addition of a new PrometheusConfig.SkipRouteEnrollment in the future.

[aldas]

oolright, I think we can and should do even further than tinkering with existing mw.

I have created PR for different take what prometheus middleware should be https://github.com/labstack/echo-contrib/pull/94

[aldas]

closing, new middleware was added with #94