promhttp: Support extra labels extracted from request context in Instrument* wrappers

[chradcliffe]

I have a use case where I'd like to be able to apply labels on a per-request basis to metrics that are collected in the Instrument* set of wrappers (e.g. InstrumentHandlerDuration). In my particular case, I have a categorization based on the User-Agent header in the request and I'd like to be able to view the stats broken down by this categorization.

I thought it might make sense to have a WithExtraLabels option that takes a func (requestCtx context.Context) prometheus.Labels to handle this use case.

This PR adds the WithExtraLabels option. I've added a few unit tests and tested it with my application. If there's an alternative option you'd like to explore, let me know and I can iterate on it.

[bwplotka]

Hi,

Thanks for this PR, I totally see the purpose. If your categorization has only few possibilities it makes total sense. However, with the proposed implementation there are to main problems:

1. It is way to easy to abuse the metric system for too dynamic label values. For less advanced users this might mean more CVEs like this in future for the services they build with this library, which is not fun. I proposed a warning, but I wonder if we should do more checks to prevent this.

2. I think there is a way to do it now, without this PR. It's less elegant, but possible, which means with this PR, there are many ways of doing the same thing. For example:

func ExampleInstrumentWithUserAgent() {
	counter := promauto.NewCounterVec(
		prometheus.CounterOpts{
			Name: "api_requests_total",
			Help: "A counter for requests to the wrapped handler.",
		},
		[]string{"code", "method", "user_agent_category"},
	)

	// Create instrumentation that inject user_agent_category label value dynamically.
	instrumentMiddlewares := func(next http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			uaCategory := "unknown"
			switch r.UserAgent() {
			case "something1":
				uaCategory = "something1"
			case "something2":
				uaCategory = "something2"
			}
			InstrumentHandlerCounter(counter.MustCurryWith(prometheus.Labels{"user_agent_category": uaCategory}), next).ServeHTTP(w, r)
		})
	}

	// Create the handlers that will be wrapped by the middleware.
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("OK"))
	})

	chain := instrumentMiddlewares(handler)

	http.Handle("/metrics", Handler())
	http.Handle("/handler", chain)

	if err := http.ListenAndServe(":3000", nil); err != nil {
		log.Fatal(err)
	}

	// Output: yolo.
}

Which works, but it is not very efficient (more allocations and some CPU overhead because of constant checks).

Another way is to pre-allocate multiple handlers, which might be a viable option:

func ExampleInstrumentWithUserAgent() {
	counter := promauto.NewCounterVec(
		prometheus.CounterOpts{
			Name: "api_requests_total",
			Help: "A counter for requests to the wrapped handler.",
		},
		[]string{"code", "method", "user_agent_category"},
	)

	// Create the handlers that will be wrapped by the middleware.
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("OK"))
	})

	uaHandlers := map[string]http.Handler{
		"something1": InstrumentHandlerCounter(counter.MustCurryWith(prometheus.Labels{"user_agent_category": "something1"}), handler),
		"something2": InstrumentHandlerCounter(counter.MustCurryWith(prometheus.Labels{"user_agent_category": "something2"}), handler),
		"default":    InstrumentHandlerCounter(counter.MustCurryWith(prometheus.Labels{"user_agent_category": "unknown"}), handler),
	}

	// Create instrumentation that inject user_agent_category label value dynamically.
	instrumentMiddlewares := func(next http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			switch r.UserAgent() {
			case "something1":
				uaHandlers["something1"].ServeHTTP(w, r)
			case "something2":
				uaHandlers["something2"].ServeHTTP(w, r)
			}
			uaHandlers["default"].ServeHTTP(w, r)
		})
	}

	chain := instrumentMiddlewares(handler)

	http.Handle("/metrics", Handler())
	http.Handle("/handler", chain)

	if err := http.ListenAndServe(":3000", nil); err != nil {
		log.Fatal(err)
	}

	// Output: yolo.
}

I can agree that both alternatives are not perfect, but we used something like this in the past. Might be good enough, given a risk involved with your PR.

I would be happy to discuss this more, maybe I am missing something here, or perhaps there is the even better solution to the semantics of the new option to make it safer to use.

[bwplotka]

Any extra labels must be unconditionally added with a reasonable default label value.

What this means? 🤔

[chradcliffe]

If the func passed to WithExtraLabels doesn't set a default value for all of the extra labels, the checkLabels function will panic. Maybe there's a better way to phrase that?

[bwplotka]

Suggested change

	// If the hook function returns non-nil labels, the labels will be added to the metrics. Any extra labels
	// If the `getExtraLabelsFn` returns non-nil labels, the labels will be added to the metrics. Any extra labels

[chradcliffe]

I'm not sure there's a clean way to do this while also limiting the cardinality of the extra labels from the library side.

I was thinking the risk (and user responsibility) of this is about the same as prometheus API code like counter.With(labels) where the user is expected to manage the cardinality of the labels. Arguably the CVE you linked to is a bit different since it was the promhttp code that was adding the label values rather than user code.

I hadn't thought of creating a map of handlers -- thanks for that. I think it will solve our immediate problem, at least for the single-label case.

In terms of this PR, I'd be interested in moving forward if others see value and we can get past the security issue, but I'm also okay with closing this since my use case can be worked around.

[bwplotka]

BTW we discus same topic in #1066 (review) and I think we are getting somewhere in terms of solution (:

[bwplotka]

#1066 (review) was merged, implementing the same. Closing this PR, thanks! Feel free to object, we can reopen.