WIP: new prometheus middleware

[aldas]

Proposal for new prometheus middleware (see #80).

This package

• separates:
  • middleware (collects metrics),
  • handler (serves collected metrics - pull)
  • pushgateway (sends collected metrics - push) parts.
• Drops all Metric struct related features you are better off creating prometheus metrics (collectors) directly and not from indirections provided by old middleware. This way we (maintainers) free ourselves of maintenance of these structs that build collectors. We hand full ownership of custom metrics to the user.
• New API does not start/run any goroutines - this is left to user.
• Pushgateway related things are completely refactored.

p.s. package name is echoprometheus because this way we get clean sheet and also it is quite annoying then you have conflicting package names (thing that we are using and middleware package).

p.s.s. naming of things needs work.

Usage (shortest form):

package main
import (
	"github.com/labstack/echo/v4"
	"github.com/labstack/echo-contrib/prometheus/echoprometheus"
)
	func main() {
	    e := echo.New()
	    // Enable metrics middleware
            e.Use(echoprometheus.NewMiddleware("myapp"))
            e.GET("/metrics", echoprometheus.NewHandler())
	    e.Logger.Fatal(e.Start(":1323"))
	}

Example for custom registry and custom metric usage:

	customRegistry := prometheus.NewRegistry()
	customCounter := prometheus.NewCounter(
		prometheus.CounterOpts{
			Name: "custom_requests_total",
			Help: "How many HTTP requests processed, partitioned by status code and HTTP method.",
		},
	)
	if err := customRegistry.Register(customCounter); err != nil {
		t.Fatal(err)
	}

	e.Use(echoprometheus.NewMiddlewareWithConfig(echoprometheus.MiddlewareConfig{
		AfterNext: func(c echo.Context, err error) {
			customCounter.Inc() // use our custom metric in middleware
		},
		Registerer: customRegistry,
	}))
	e.GET("/metrics", echoprometheus.NewHandlerWithConfig(echoprometheus.HandlerConfig{Gatherer: customRegistry}))

that custom metric will appear in output as

	assert.Contains(t, body, `custom_requests_total 1`)

Example for custom labels for middleware default metrics:

	e.Use(echoprometheus.NewMiddlewareWithConfig(echoprometheus.MiddlewareConfig{
		LabelFuncs: map[string]echoprometheus.LabelValueFunc{
			"scheme": func(c echo.Context, err error) string { // additional custom label
				return c.Scheme()
			},
			"method": func(c echo.Context, err error) string { // overrides default 'method' label value
				return "overridden_" + c.Request().Method
			},
		},
	}))
	e.GET("/metrics", echoprometheus.NewHandler())

Example for changing middleware default metrics options before registering metrics:

	e := echo.New()
	e.Use(echoprometheus.NewMiddlewareWithConfig(echoprometheus.MiddlewareConfig{
		HistogramOptsFunc: func(opts prometheus.HistogramOpts) prometheus.HistogramOpts {
			if opts.Name == "request_duration_seconds" {
				opts.ConstLabels = prometheus.Labels{"my_const": "123"}
				opts.Buckets = prometheus.DefBuckets
			}
			return opts
		},
	}))

Example running push gateway goroutine

go func() {
	config := echoprometheus.PushGatewayConfig{
		PushGatewayURL: "https://host:9080",
		PushInterval:   10 * time.Millisecond,
	}
	if err := echoprometheus.RunPushGatewayGatherer(context.Background(), config); !errors.Is(err, context.Canceled) {
		log.Fatal(err)
	}
}()

[codecov]

Codecov Report

Patch coverage: 77.16% and project coverage change: +4.97 🎉

Comparison is base (5bb0f68) 58.79% compared to head (2b29ab5) 63.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   58.79%   63.77%   +4.97%     
==========================================
  Files           8        9       +1     
  Lines         682      944     +262     
==========================================
+ Hits          401      602     +201     
- Misses        258      302      +44     
- Partials       23       40      +17     

Impacted Files	Coverage Δ
prometheus/prometheus.go	49.15% <ø> (ø)
echoprometheus/prometheus.go	77.16% <77.16%> (ø)

... and 1 file with indirect coverage changes

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

[aldas]

@lammel your thoughts?

[yabberyabber]

The API you're putting forward looks really good to me. I especially like the way you expose BeforeNext and AfterNext which seems like an obvious hook with which to implement most types of data calculation.

I left a few nitty comments about implementation details although I have to say I'm very fond of the public API being exposed here.

A few quick questions

• Do you have a model in mind for whether/how the BeforeNext can create some context that gets referenced by the AfterNext? The example I have in mind is whether/how we could have implemented the elapsed time calculation outside of the API boundary. I see that there is already an elapsed time metric internally but I'm just curious to see whether something similar to this would be possible from outside the API boundary
• Does the customMetrics API get a lot of usage out of customers right now? Are you aware of any specific customMetric implementations that need to be supported? Looking at the recent issues and PRs around it, I got the sense that it might not have been working until relatively recently. I'm wondering if maybe version 1 of this middleware aught to not support this functionality. Then the exact API can be developed later on once we have specific use-cases in mind.

[lammel]

Does this have to be public? I guess most use cases are using NewMiddlewareWithConfig anyway

[aldas]

This is to allow handling problems with middleware creation without panics. Similar to https://github.com/labstack/echo-jwt/blob/95b0b607987a3bed484870b2052ef212763742a4/jwt.go#L173

[lammel]

@lammel your thoughts?

For background
We were using our own custom middleware for prometheus metrics which we are currently refactoring to use https://github.com/VictoriaMetrics/metrics which is far less bloated and has fewer dependecies.
Due to it's simplicity we added a helper that will build metrics names to have a similar usage that prometheus.

Feedback - What comes to my mind

• Can we make both compatible, so the old NewPrometheus would just use the new implementation at least for the basic use case. This would allow to continue in the current namespace with a compatibility layer that will be marked deprecated
• Is the pushgateway functionality required (it is quite easy to setup a pushgateway which is normaly required for other metrics of the same node)
• Should we switch to https://github.com/VictoriaMetrics/metrics if it's a new module anyway (this is somewhat offttopic, as we could simply add an echo-contrib/metrics package)

About the PR

• Separation of concers for middleware, handler and pushgateway is great
• Not sure if HistogramOptsFunc helper is required, 90% of the use case is probably to override bucketsize, but it's optional and very flexible

For the way forward I'm thinking of what @yabberyabber probably referred to

func NewPrometheus(subsystem string, skipper middleware.Skipper, customMetricsList ...[]*Metric) *Prometheus {}

Making this compatible with the new implementation could be made working if we simply ignore the customMetricsList or adding to the Registerer. The current implementation is rather awkward, so I guess the best way for developers would be to only have the simplified function for compatibility and fail-fast in case someone is using customMetricsList

func NewPrometheus(subsystem string, skipper middleware.Skipper) *Prometheus {}

[aldas]

@lammel

About package name:

  customRegistry := prometheus.NewRegistry()
  customCounter := prometheus.NewCounter(
    prometheus.CounterOpts{
      Name: "custom_requests_total",
      Help: "How many HTTP requests processed, partitioned by status code and HTTP method.",
    },
  )
  if err := customRegistry.Register(customCounter); err != nil {
    t.Fatal(err)
  }

  e.Use(echoprometheus.NewMiddlewareWithConfig(echoprometheus.MiddlewareConfig{
    AfterNext: func(c echo.Context, err error) {
      customCounter.Inc() // use our custom metric in middleware
    },
    Registerer: customRegistry,
  }))
  e.GET("/metrics", echoprometheus.NewHandlerWithConfig(echoprometheus.HandlerConfig{Gatherer: customRegistry}))

is good example why having middleware in prometheus package is annoying. When you need to access actual Prometheus stuff you have conflicting package names and need to alias which is inconvient and you still end up situation where you probably alias middleware package name to something other and leave Prometheus own package name as is.

This is similar to JWT middleware situation.

---

Can we make both compatible, so the old NewPrometheus would just use the new implementation at least for the basic use case. This would allow to continue in the current namespace with a compatibility layer that will be marked deprecated

Making this compatible with the new implementation could be made working if we simply ignore the customMetricsList or adding to the Registerer.

I really not think we should force this newer API on older API or hack these two to work together. It would be confusing as new API is conceptionally different - it has separated concerns where older API has Prometheus struct that is middleware, handler(pull), sender (push) all at the same time.

About current Metric struct

echo-contrib/prometheus/prometheus.go

Line 130 in b6855c2

type Metric struct {

I think it started out as a abstraction so middleware API would not be too tied to Prometheus own API but as because of MetricCollector prometheus.Collector field this is futile attempt.
Also - having these wrappers/abstractios for use maitenance burden (this Metric should be able todo same things as native Prometheus Collectors API) and sametime it limits user freedom.

Dropping all of this logic/abstrction from middleware package resposibilities moves all resposibilities to the user.

echo-contrib/prometheus/prometheus.go

Line 300 in b6855c2

func NewMetric(m *Metric, subsystem string) prometheus.Collector {

logic like that is not needed.

To be able to create collectors as you see fit and use them in Middleware with BeforeNext or AfterNext callback is superior in freedom/flexibility.

---

Is the pushgateway functionality required (it is quite easy to setup a pushgateway which is normaly required for other metrics of the same node)

It is probably helpful.

which is normaly required for other metrics of the same node

I do not understand what you mean by that? In case you are creating custom collectors (metrics) for your background processes etc, you can register them with your prometheus.Registerer or prometheus.DefaultRegistry instance and pull/push them with your HTTP+GO etc related metrics.

---

Not sure if HistogramOptsFunc helper is required, 90% of the use case is probably to override bucketsize, but it's optional and very flexible

There are issues where people want to change buckets, namespace etc. It felt like most flexible way to allow users control over how default collectors/metrics are created. With that function you can change single one or all that pass through.

---

Should we switch to https://github.com/VictoriaMetrics/metrics if it's a new module anyway (this is somewhat offttopic, as we could simply add an echo-contrib/metrics package)

I think separate package would be better. Currently new API and old Prometheus MW API is atleast compatible when it comes to Prometheus own package structs/interfaces. Direct leap from Prometheus lib to VictoriaMetrics implementation is probably that takes time. Going from old API to new API is much easier when you do not do fancy things.

[aldas]

@yabberyabber

Do you have a model in mind for whether/how the BeforeNext can create some context that gets referenced by the AfterNext? The example I have in mind is whether/how we could have implemented the elapsed time calculation outside of the API boundary. I see that there is already an elapsed time metric internally but I'm just curious to see whether something similar to this would be possible from outside the API boundary.

I thought about it - for example we could have some object created at the start of request and it implements Before and After methods
and could carry state between these two calls inside that object, but this is probably way to complex/inconvenient to use.

Currently if you want to pass some "state" from BeforeNext to AfterNext you can set values to echo.Context with c.Set("was_before", true) and get it from context store with wasBefore, ok := c.Get("was_before").(bool)

---

Does the customMetrics API get a lot of usage out of customers right now? Are you aware of any specific customMetric implementations that need to be supported? Looking at the recent issues and PRs around it, I got the sense that it might not have been working until relatively recently. I'm wondering if maybe version 1 of this middleware aught to not support this functionality. Then the exact API can be developed later on once we have specific use-cases in mind.

I do not actually know what exactly are you meaning by customMetrics API. If you mean current Metric struct and functions related to that.

I do not think it should exists. Creation and usage of custom collector should be left to the developer. If we are talking about collectors/metrics related to HTTP requests then BeforeNext to AfterNext is the API which allows developer that freedom.

If we are talking about these default metrics HTTP middleware registers and customMetricsList allows to "overwrite" default ones - newer API counterpart for that is MiddlewareConfig.HistogramOptsFunc and MiddlewareConfig.CounterOptsFunc

I really think

echo-contrib/prometheus/prometheus.go

Line 300 in b6855c2

func NewMetric(m *Metric, subsystem string) prometheus.Collector {

is unnecessary complexity and maintenance burden.

If it is about other custom collectors (maybe you have background processes that you meter). Then you would just register these collectors/metrics to your custom Registry or use prometheus.DefaultRegistry and see them outputted from handler or pushgateway - assuming these are configured to use that Registry where your custom collectors/metrics were registered.

[lammel]

@aldas

About package name:

is good example why having middleware in prometheus package is annoying. When you need to access actual Prometheus stuff you have conflicting package names and need to alias which is inconvient and you still end up situation where you probably alias middleware package name to something other and leave Prometheus own package name as is.

Although aliasing for import works quite well I see your point. Conflicting package names can cause some headache.
But in that case I'd prefer to create a completely new echoprometheus middleware (not under prometheus directly in the echo-contrib root. That way it is quite clear, that no compatibility is expected, we can add short section "How to migrate from old prometheus middleware" to the README

Is the pushgateway functionality required (it is quite easy to setup a pushgateway which is normaly required for other metrics of the same node)

It is probably helpful.

Ok, keep it.

which is normaly required for other metrics of the same node

I do not understand what you mean by that? In case you are creating custom collectors (metrics) for your background processes etc, you can register them with your prometheus.Registerer or prometheus.DefaultRegistry instance and pull/push them with your HTTP+GO etc related metrics.

I was referring to the deployment on servers, where a go daemon is usually just a small part of the puzzle. If push is required for metrics there is probably already a pushgateway running somewhere near to also push other node metrics (system metrics, other applications, ...). But as above, just keep it, as it's there already.

Not sure if HistogramOptsFunc helper is required, 90% of the use case is probably to override bucketsize, but it's optional and very flexible

There are issues where people want to change buckets, namespace etc. It felt like most flexible way to allow users control over how default collectors/metrics are created. With that function you can change single one or all that pass through.

Understood, I agree this is the must flexible option we have.

Should we switch to https://github.com/VictoriaMetrics/metrics if it's a new module anyway (this is somewhat offttopic, as we could simply add an echo-contrib/metrics package)

I think separate package would be better. Currently new API and old Prometheus MW API is atleast compatible when it comes to Prometheus own package structs/interfaces. Direct leap from Prometheus lib to VictoriaMetrics implementation is probably that takes time. Going from old API to new API is much easier when you do not do fancy things.

I completely agree.

So to summarize:

• All new echoprometheus middleware is the preferred way
• Just move from prometheus/echoprometheus to echoprometheus
• No compatibility to older prometheus middleware required or advertised
• No additional wrappers for other application metrics required (just use prometheus library)
• Short "How to migrate" section in a echoprometheus/README.md would be helpful

[yabberyabber]

I do not actually know what exactly are you meaning by customMetrics API. If you mean current Metric struct and functions related to that.

I do not think it should exists. Creation and usage of custom collector should be left to the developer. If we are talking about collectors/metrics related to HTTP requests then BeforeNext to AfterNext is the API which allows developer that freedom.

I guess what I'm trying to say is do we need to provide the BeforeNext and AfterNext hooks at all? Is there a specific behavior that we need to support that these enable? My suggestion is maybe we omit them for now and wait to see who complains.

[yabberyabber]

This API as written satisfies my usecase and I think is much easier to use than the previous API. Thanks for putting this together 👍 Let me know if there's anything I can do on my end to help you get this in. I did a brief walk through and I only had a couple small comments.

[yabberyabber]

Hey @aldas any update on this? I've got a project building off your fork and I would love to be able to point my go.mod replace rule at the upstream repo :)

Is there anything I can do to help this PR along?

[aldas]

@yabberyabber If you are in a hurry and want to help - could you copy this middleware code at the moment in your application and start to use it - this would give us practical information how it works in real world. If you are worried about licencing - this is MIT so just add the header and you are good.

I'll fix @lammel review comments and try to get this PR finished this week.

[aldas]

@lammel please re-review.

Mostly addresses these points from previous review

• Just move from prometheus/echoprometheus to echoprometheus
• Short "How to migrate" section in a echoprometheus/README.md would be helpful

and you know - I am no Shakespeare. so there is room for improvement in migration section.

[lammel]

@lammel please re-review.

Mostly addresses these points from previous review

• Just move from prometheus/echoprometheus to echoprometheus
• Short "How to migrate" section in a echoprometheus/README.md would be helpful

and you know - I am no Shakespeare. so there is room for improvement in migration section.

Great! Just need some time to review. Give me a few days.

[yabberyabber]

Thanks @aldas ! I just rebased my local projects to use the new middleware and it has been working great.

I was just about to suggest you put the Deprectaed annotation on the old prometheus middleware but I see you've already done that :) That'll go a long way in helping current users to transition.

[lammel]

Looks good!
Only some minor textual suggestions.

[aldas]

all right, textual changes done. @lammel, your approvement please and I'll merge and create next release.

[lammel]

LGTM!

[aldas]

Thanks @yabberyabber and @lammel. I'll update Echo website soon