From eb891d46831252c5329218bfbb606727685fea72 Mon Sep 17 00:00:00 2001 From: Dave Henderson Date: Sat, 22 Jan 2022 19:08:57 -0500 Subject: [PATCH 1/4] metrics: Enforce smaller set of method labels Signed-off-by: Dave Henderson --- metrics.go | 27 +++++++++++++++++++++++++-- modules/caddyhttp/metrics.go | 27 +++++++++++++++++++++++++-- modules/caddyhttp/metrics_test.go | 23 +++++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/metrics.go b/metrics.go index ab9d7978..9a56f733 100644 --- a/metrics.go +++ b/metrics.go @@ -3,7 +3,6 @@ package caddy import ( "net/http" "strconv" - "strings" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/collectors" @@ -47,7 +46,7 @@ func instrumentHandlerCounter(counter *prometheus.CounterVec, next http.Handler) next.ServeHTTP(d, r) counter.With(prometheus.Labels{ "code": sanitizeCode(d.status), - "method": strings.ToUpper(r.Method), + "method": sanitizeMethod(r.Method), }).Inc() }) } @@ -76,3 +75,27 @@ func sanitizeCode(s int) string { return strconv.Itoa(s) } } + +// Only support the list of "regular" HTTP methods, see +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods +var methodMap = map[string]string{ + "GET": http.MethodGet, "get": http.MethodGet, + "HEAD": http.MethodHead, "head": http.MethodHead, + "PUT": http.MethodPut, "put": http.MethodPut, + "POST": http.MethodPost, "post": http.MethodPost, + "DELETE": http.MethodDelete, "delete": http.MethodDelete, + "CONNECT": http.MethodConnect, "connect": http.MethodConnect, + "OPTIONS": http.MethodOptions, "options": http.MethodOptions, + "TRACE": http.MethodTrace, "trace": http.MethodTrace, + "PATCH": http.MethodPatch, "patch": http.MethodPatch, +} + +// sanitizeMethod sanitizes the method for use as a metric label. This helps +// prevent high cardinality on the method label. The name is always upper case. +func sanitizeMethod(m string) string { + if m, ok := methodMap[m]; ok { + return m + } + + return "other" +} diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index 3e5d6396..8aa91846 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -4,7 +4,6 @@ import ( "context" "net/http" "strconv" - "strings" "sync" "time" @@ -109,7 +108,7 @@ func newMetricsInstrumentedHandler(handler string, mh MiddlewareHandler) *metric func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next Handler) error { server := serverNameFromContext(r.Context()) labels := prometheus.Labels{"server": server, "handler": h.handler} - method := strings.ToUpper(r.Method) + method := sanitizeMethod(r.Method) // the "code" value is set later, but initialized here to eliminate the possibility // of a panic statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": method, "code": ""} @@ -160,6 +159,30 @@ func sanitizeCode(code int) string { return strconv.Itoa(code) } +// Only support the list of "regular" HTTP methods, see +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods +var methodMap = map[string]string{ + "GET": http.MethodGet, "get": http.MethodGet, + "HEAD": http.MethodHead, "head": http.MethodHead, + "PUT": http.MethodPut, "put": http.MethodPut, + "POST": http.MethodPost, "post": http.MethodPost, + "DELETE": http.MethodDelete, "delete": http.MethodDelete, + "CONNECT": http.MethodConnect, "connect": http.MethodConnect, + "OPTIONS": http.MethodOptions, "options": http.MethodOptions, + "TRACE": http.MethodTrace, "trace": http.MethodTrace, + "PATCH": http.MethodPatch, "patch": http.MethodPatch, +} + +// sanitizeMethod sanitizes the method for use as a metric label. This helps +// prevent high cardinality on the method label. The name is always upper case. +func sanitizeMethod(m string) string { + if m, ok := methodMap[m]; ok { + return m + } + + return "other" +} + // taken from https://github.com/prometheus/client_golang/blob/6007b2b5cae01203111de55f753e76d8dac1f529/prometheus/promhttp/instrument_server.go#L298 func computeApproximateRequestSize(r *http.Request) int { s := 0 diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go index 6311935a..78e380b6 100644 --- a/modules/caddyhttp/metrics_test.go +++ b/modules/caddyhttp/metrics_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "github.com/prometheus/client_golang/prometheus/testutil" @@ -82,3 +83,25 @@ type middlewareHandlerFunc func(http.ResponseWriter, *http.Request, Handler) err func (f middlewareHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request, h Handler) error { return f(w, r, h) } + +func TestSanitizeMethod(t *testing.T) { + tests := []struct { + method string + expected string + }{ + {method: "get", expected: "GET"}, + {method: "POST", expected: "POST"}, + {method: "OPTIONS", expected: "OPTIONS"}, + {method: "connect", expected: "CONNECT"}, + {method: "trace", expected: "TRACE"}, + {method: "UNKNOWN", expected: "other"}, + {method: strings.Repeat("ohno", 9999), expected: "other"}, + } + + for _, d := range tests { + actual := sanitizeMethod(d.method) + if actual != d.expected { + t.Errorf("Not same: expected %#v, but got %#v", d.expected, actual) + } + } +} From 042abeb431c55a12cc101efdb75b1c1b67340cb4 Mon Sep 17 00:00:00 2001 From: Dave Henderson Date: Sat, 22 Jan 2022 19:30:16 -0500 Subject: [PATCH 2/4] other is not uppercase Signed-off-by: Dave Henderson --- metrics.go | 2 +- modules/caddyhttp/metrics.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/metrics.go b/metrics.go index 9a56f733..8a502601 100644 --- a/metrics.go +++ b/metrics.go @@ -97,5 +97,5 @@ func sanitizeMethod(m string) string { return m } - return "other" + return "OTHER" } diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index 8aa91846..f2023cf2 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -180,7 +180,7 @@ func sanitizeMethod(m string) string { return m } - return "other" + return "OTHER" } // taken from https://github.com/prometheus/client_golang/blob/6007b2b5cae01203111de55f753e76d8dac1f529/prometheus/promhttp/instrument_server.go#L298 From da4a759bad70cae9f4f1f38a03c26f75aa04352f Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sun, 23 Jan 2022 01:17:14 -0500 Subject: [PATCH 3/4] Update modules/caddyhttp/metrics_test.go --- modules/caddyhttp/metrics_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go index 78e380b6..95f6d9b6 100644 --- a/modules/caddyhttp/metrics_test.go +++ b/modules/caddyhttp/metrics_test.go @@ -94,8 +94,8 @@ func TestSanitizeMethod(t *testing.T) { {method: "OPTIONS", expected: "OPTIONS"}, {method: "connect", expected: "CONNECT"}, {method: "trace", expected: "TRACE"}, - {method: "UNKNOWN", expected: "other"}, - {method: strings.Repeat("ohno", 9999), expected: "other"}, + {method: "UNKNOWN", expected: "OTHER"}, + {method: strings.Repeat("ohno", 9999), expected: "OTHER"}, } for _, d := range tests { From 7ca5921a87c819f9848ccd7ec786aab0f896be72 Mon Sep 17 00:00:00 2001 From: Dave Henderson Date: Mon, 24 Jan 2022 08:35:51 -0500 Subject: [PATCH 4/4] move common metrics-related funcs to internal package Signed-off-by: Dave Henderson --- internal/metrics/metrics.go | 39 +++++++++++++++++++++++++++++++ internal/metrics/metrics_test.go | 28 ++++++++++++++++++++++ metrics.go | 39 +++---------------------------- modules/caddyhttp/metrics.go | 39 ++++--------------------------- modules/caddyhttp/metrics_test.go | 23 ------------------ 5 files changed, 74 insertions(+), 94 deletions(-) create mode 100644 internal/metrics/metrics.go create mode 100644 internal/metrics/metrics_test.go diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 00000000..7ae09b60 --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,39 @@ +package metrics + +import ( + "net/http" + "strconv" +) + +func SanitizeCode(s int) string { + switch s { + case 0, 200: + return "200" + default: + return strconv.Itoa(s) + } +} + +// Only support the list of "regular" HTTP methods, see +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods +var methodMap = map[string]string{ + "GET": http.MethodGet, "get": http.MethodGet, + "HEAD": http.MethodHead, "head": http.MethodHead, + "PUT": http.MethodPut, "put": http.MethodPut, + "POST": http.MethodPost, "post": http.MethodPost, + "DELETE": http.MethodDelete, "delete": http.MethodDelete, + "CONNECT": http.MethodConnect, "connect": http.MethodConnect, + "OPTIONS": http.MethodOptions, "options": http.MethodOptions, + "TRACE": http.MethodTrace, "trace": http.MethodTrace, + "PATCH": http.MethodPatch, "patch": http.MethodPatch, +} + +// SanitizeMethod sanitizes the method for use as a metric label. This helps +// prevent high cardinality on the method label. The name is always upper case. +func SanitizeMethod(m string) string { + if m, ok := methodMap[m]; ok { + return m + } + + return "OTHER" +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go new file mode 100644 index 00000000..c3f5965b --- /dev/null +++ b/internal/metrics/metrics_test.go @@ -0,0 +1,28 @@ +package metrics + +import ( + "strings" + "testing" +) + +func TestSanitizeMethod(t *testing.T) { + tests := []struct { + method string + expected string + }{ + {method: "get", expected: "GET"}, + {method: "POST", expected: "POST"}, + {method: "OPTIONS", expected: "OPTIONS"}, + {method: "connect", expected: "CONNECT"}, + {method: "trace", expected: "TRACE"}, + {method: "UNKNOWN", expected: "OTHER"}, + {method: strings.Repeat("ohno", 9999), expected: "OTHER"}, + } + + for _, d := range tests { + actual := SanitizeMethod(d.method) + if actual != d.expected { + t.Errorf("Not same: expected %#v, but got %#v", d.expected, actual) + } + } +} diff --git a/metrics.go b/metrics.go index 8a502601..325006fb 100644 --- a/metrics.go +++ b/metrics.go @@ -2,8 +2,8 @@ package caddy import ( "net/http" - "strconv" + "github.com/caddyserver/caddy/v2/internal/metrics" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/collectors" "github.com/prometheus/client_golang/prometheus/promauto" @@ -45,8 +45,8 @@ func instrumentHandlerCounter(counter *prometheus.CounterVec, next http.Handler) d := newDelegator(w) next.ServeHTTP(d, r) counter.With(prometheus.Labels{ - "code": sanitizeCode(d.status), - "method": sanitizeMethod(r.Method), + "code": metrics.SanitizeCode(d.status), + "method": metrics.SanitizeMethod(r.Method), }).Inc() }) } @@ -66,36 +66,3 @@ func (d *delegator) WriteHeader(code int) { d.status = code d.ResponseWriter.WriteHeader(code) } - -func sanitizeCode(s int) string { - switch s { - case 0, 200: - return "200" - default: - return strconv.Itoa(s) - } -} - -// Only support the list of "regular" HTTP methods, see -// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods -var methodMap = map[string]string{ - "GET": http.MethodGet, "get": http.MethodGet, - "HEAD": http.MethodHead, "head": http.MethodHead, - "PUT": http.MethodPut, "put": http.MethodPut, - "POST": http.MethodPost, "post": http.MethodPost, - "DELETE": http.MethodDelete, "delete": http.MethodDelete, - "CONNECT": http.MethodConnect, "connect": http.MethodConnect, - "OPTIONS": http.MethodOptions, "options": http.MethodOptions, - "TRACE": http.MethodTrace, "trace": http.MethodTrace, - "PATCH": http.MethodPatch, "patch": http.MethodPatch, -} - -// sanitizeMethod sanitizes the method for use as a metric label. This helps -// prevent high cardinality on the method label. The name is always upper case. -func sanitizeMethod(m string) string { - if m, ok := methodMap[m]; ok { - return m - } - - return "OTHER" -} diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index f2023cf2..458c22af 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -3,10 +3,10 @@ package caddyhttp import ( "context" "net/http" - "strconv" "sync" "time" + "github.com/caddyserver/caddy/v2/internal/metrics" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" ) @@ -108,7 +108,7 @@ func newMetricsInstrumentedHandler(handler string, mh MiddlewareHandler) *metric func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next Handler) error { server := serverNameFromContext(r.Context()) labels := prometheus.Labels{"server": server, "handler": h.handler} - method := sanitizeMethod(r.Method) + method := metrics.SanitizeMethod(r.Method) // the "code" value is set later, but initialized here to eliminate the possibility // of a panic statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": method, "code": ""} @@ -123,7 +123,7 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re // being called when the headers are written. // Effectively the same behaviour as promhttp.InstrumentHandlerTimeToWriteHeader. writeHeaderRecorder := ShouldBufferFunc(func(status int, header http.Header) bool { - statusLabels["code"] = sanitizeCode(status) + statusLabels["code"] = metrics.SanitizeCode(status) ttfb := time.Since(start).Seconds() httpMetrics.responseDuration.With(statusLabels).Observe(ttfb) return false @@ -142,7 +142,7 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re if statusLabels["code"] == "" { // we still sanitize it, even though it's likely to be 0. A 200 is // returned on fallthrough so we want to reflect that. - statusLabels["code"] = sanitizeCode(wrec.Status()) + statusLabels["code"] = metrics.SanitizeCode(wrec.Status()) } httpMetrics.requestDuration.With(statusLabels).Observe(dur) @@ -152,37 +152,6 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re return nil } -func sanitizeCode(code int) string { - if code == 0 { - return "200" - } - return strconv.Itoa(code) -} - -// Only support the list of "regular" HTTP methods, see -// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods -var methodMap = map[string]string{ - "GET": http.MethodGet, "get": http.MethodGet, - "HEAD": http.MethodHead, "head": http.MethodHead, - "PUT": http.MethodPut, "put": http.MethodPut, - "POST": http.MethodPost, "post": http.MethodPost, - "DELETE": http.MethodDelete, "delete": http.MethodDelete, - "CONNECT": http.MethodConnect, "connect": http.MethodConnect, - "OPTIONS": http.MethodOptions, "options": http.MethodOptions, - "TRACE": http.MethodTrace, "trace": http.MethodTrace, - "PATCH": http.MethodPatch, "patch": http.MethodPatch, -} - -// sanitizeMethod sanitizes the method for use as a metric label. This helps -// prevent high cardinality on the method label. The name is always upper case. -func sanitizeMethod(m string) string { - if m, ok := methodMap[m]; ok { - return m - } - - return "OTHER" -} - // taken from https://github.com/prometheus/client_golang/blob/6007b2b5cae01203111de55f753e76d8dac1f529/prometheus/promhttp/instrument_server.go#L298 func computeApproximateRequestSize(r *http.Request) int { s := 0 diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go index 95f6d9b6..6311935a 100644 --- a/modules/caddyhttp/metrics_test.go +++ b/modules/caddyhttp/metrics_test.go @@ -5,7 +5,6 @@ import ( "errors" "net/http" "net/http/httptest" - "strings" "testing" "github.com/prometheus/client_golang/prometheus/testutil" @@ -83,25 +82,3 @@ type middlewareHandlerFunc func(http.ResponseWriter, *http.Request, Handler) err func (f middlewareHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request, h Handler) error { return f(w, r, h) } - -func TestSanitizeMethod(t *testing.T) { - tests := []struct { - method string - expected string - }{ - {method: "get", expected: "GET"}, - {method: "POST", expected: "POST"}, - {method: "OPTIONS", expected: "OPTIONS"}, - {method: "connect", expected: "CONNECT"}, - {method: "trace", expected: "TRACE"}, - {method: "UNKNOWN", expected: "OTHER"}, - {method: strings.Repeat("ohno", 9999), expected: "OTHER"}, - } - - for _, d := range tests { - actual := sanitizeMethod(d.method) - if actual != d.expected { - t.Errorf("Not same: expected %#v, but got %#v", d.expected, actual) - } - } -}