emptyLabels used in instrument_server.go cause concurrent map writes #1274
Description
When using dynamic labels from context on metrics that have no labels the same map is used for all handlers. To illustrate here is the InstrumentHandlerDuration function:
func InstrumentHandlerDuration(obs prometheus.ObserverVec, next http.Handler, opts ...Option) http.HandlerFunc {
hOpts := defaultOptions()
for _, o := range opts {
o.apply(hOpts)
}
// Curry the observer with dynamic labels before checking the remaining labels.
code, method := checkLabels(obs.MustCurryWith(hOpts.emptyDynamicLabels()))
if code {
return func(w http.ResponseWriter, r *http.Request) {
now := time.Now()
d := newDelegator(w, nil)
next.ServeHTTP(d, r)
l := labels(code, method, r.Method, d.Status(), hOpts.extraMethods...)
for label, resolve := range hOpts.extraLabelsFromCtx {
l[label] = resolve(r.Context())
}
observeWithExemplar(obs.With(l), time.Since(now).Seconds(), hOpts.getExemplarFn(r.Context()))
}
}
return func(w http.ResponseWriter, r *http.Request) {
now := time.Now()
next.ServeHTTP(w, r)
l := labels(code, method, r.Method, 0, hOpts.extraMethods...)
for label, resolve := range hOpts.extraLabelsFromCtx {
l[label] = resolve(r.Context())
}
observeWithExemplar(obs.With(l), time.Since(now).Seconds(), hOpts.getExemplarFn(r.Context()))
}
}The issue arises when both code and method are false. Within the labels function:
// emptyLabels is a one-time allocation for non-partitioned metrics to avoid
// unnecessary allocations on each request.
var emptyLabels = prometheus.Labels{}
func labels(code, method bool, reqMethod string, status int, extraMethods ...string) prometheus.Labels {
if !(code || method) {
return emptyLabels
}
labels := prometheus.Labels{}
if code {
labels["code"] = sanitizeCode(status)
}
if method {
labels["method"] = sanitizeMethod(reqMethod, extraMethods...)
}
return labels
}if both code and method are false the emptyLabels map is returned.
the emptyLabels map is only allocated once as can seen by the comment.
After it is returned, the map is then filled with dynamic labels in the following code segment:
l := labels(code, method, r.Method, 0, hOpts.extraMethods...)
for label, resolve := range hOpts.extraLabelsFromCtx {
l[label] = resolve(r.Context())
}This fills the emptyLabels map with values, which is a bug in and of itself, but the real issue arises when there are two handlers that reach this code. When two handlers both try to write to the same map it results in panic: concurrent map writes.
Suggested fix (simplest): Let's allocate the empty map every time instead of once, i.e.:
func labels(code, method bool, reqMethod string, status int, extraMethods ...string) prometheus.Labels {
labels := prometheus.Labels{}
if code {
labels["code"] = sanitizeCode(status)
}
if method {
labels["method"] = sanitizeMethod(reqMethod, extraMethods...)
}
return labels
}