From 1d1b6c3199df9bdebc5dcaeb33b0e319b6c4a163 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Mon, 27 May 2024 16:46:27 +1000 Subject: [PATCH 1/3] Extract route name logic to a single middleware and use it everywhere. --- middleware/grpc_stats.go | 1 + middleware/http_tracing.go | 13 ++--- middleware/instrument.go | 51 +---------------- middleware/instrument_test.go | 34 ----------- middleware/route_injector.go | 95 +++++++++++++++++++++++++++++++ middleware/route_injector_test.go | 71 +++++++++++++++++++++++ server/server.go | 7 ++- 7 files changed, 177 insertions(+), 95 deletions(-) delete mode 100644 middleware/instrument_test.go create mode 100644 middleware/route_injector.go create mode 100644 middleware/route_injector_test.go diff --git a/middleware/grpc_stats.go b/middleware/grpc_stats.go index 3d29d9baa..ec766d640 100644 --- a/middleware/grpc_stats.go +++ b/middleware/grpc_stats.go @@ -31,6 +31,7 @@ type contextKey int const ( contextKeyMethodName contextKey = 1 + contextKeyRouteName contextKey = 2 ) func (g *grpcStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context { diff --git a/middleware/http_tracing.go b/middleware/http_tracing.go index 901970a4a..b2c34d114 100644 --- a/middleware/http_tracing.go +++ b/middleware/http_tracing.go @@ -24,14 +24,13 @@ var _ = nethttp.MWURLTagFunc // Tracer is a middleware which traces incoming requests. type Tracer struct { - RouteMatcher RouteMatcher - SourceIPs *SourceIPExtractor + SourceIPs *SourceIPExtractor } // Wrap implements Interface func (t Tracer) Wrap(next http.Handler) http.Handler { options := []nethttp.MWOption{ - nethttp.OperationNameFunc(makeHTTPOperationNameFunc(t.RouteMatcher)), + nethttp.OperationNameFunc(httpOperationNameFunc), nethttp.MWSpanObserver(func(sp opentracing.Span, r *http.Request) { // add a tag with the client's user agent to the span userAgent := r.Header.Get("User-Agent") @@ -130,11 +129,9 @@ func HTTPGRPCTracingInterceptor(router *mux.Router) grpc.UnaryServerInterceptor } } -func makeHTTPOperationNameFunc(routeMatcher RouteMatcher) func(r *http.Request) string { - return func(r *http.Request) string { - routeName := getRouteName(routeMatcher, r) - return getOperationName(routeName, r) - } +func httpOperationNameFunc(r *http.Request) string { + routeName := ExtractRouteName(r) + return getOperationName(routeName, r) } func getOperationName(routeName string, r *http.Request) string { diff --git a/middleware/instrument.go b/middleware/instrument.go index 6e821e4c0..098fc9a05 100644 --- a/middleware/instrument.go +++ b/middleware/instrument.go @@ -8,7 +8,6 @@ import ( "context" "io" "net/http" - "regexp" "strconv" "strings" @@ -45,7 +44,6 @@ func (f PerTenantCallback) shouldInstrument(ctx context.Context) (string, bool) // Instrument is a Middleware which records timings for every HTTP request type Instrument struct { - RouteMatcher RouteMatcher Duration *prometheus.HistogramVec PerTenantDuration *prometheus.HistogramVec PerTenantCallback PerTenantCallback @@ -120,7 +118,7 @@ func (i Instrument) Wrap(next http.Handler) http.Handler { // We do all this as we do not wish to emit high cardinality labels to // prometheus. func (i Instrument) getRouteName(r *http.Request) string { - route := getRouteName(i.RouteMatcher, r) + route := ExtractRouteName(r) if route == "" { route = "other" } @@ -128,53 +126,6 @@ func (i Instrument) getRouteName(r *http.Request) string { return route } -func getRouteName(routeMatcher RouteMatcher, r *http.Request) string { - var routeMatch mux.RouteMatch - if routeMatcher == nil || !routeMatcher.Match(r, &routeMatch) { - return "" - } - - if routeMatch.MatchErr == mux.ErrNotFound { - return "notfound" - } - - if routeMatch.Route == nil { - return "" - } - - if name := routeMatch.Route.GetName(); name != "" { - return name - } - - tmpl, err := routeMatch.Route.GetPathTemplate() - if err == nil { - return MakeLabelValue(tmpl) - } - - return "" -} - -var invalidChars = regexp.MustCompile(`[^a-zA-Z0-9]+`) - -// MakeLabelValue converts a Gorilla mux path to a string suitable for use in -// a Prometheus label value. -func MakeLabelValue(path string) string { - // Convert non-alnums to underscores. - result := invalidChars.ReplaceAllString(path, "_") - - // Trim leading and trailing underscores. - result = strings.Trim(result, "_") - - // Make it all lowercase - result = strings.ToLower(result) - - // Special case. - if result == "" { - result = "root" - } - return result -} - type reqBody struct { b io.ReadCloser read int64 diff --git a/middleware/instrument_test.go b/middleware/instrument_test.go deleted file mode 100644 index 23ab4e5b0..000000000 --- a/middleware/instrument_test.go +++ /dev/null @@ -1,34 +0,0 @@ -// Provenance-includes-location: https://github.com/weaveworks/common/blob/main/middleware/instrument_test.go -// Provenance-includes-license: Apache-2.0 -// Provenance-includes-copyright: Weaveworks Ltd. - -package middleware_test - -import ( - "testing" - - "github.com/grafana/dskit/middleware" -) - -func TestMakeLabelValue(t *testing.T) { - for input, want := range map[string]string{ - "/": "root", // special case - "//": "root", // unintended consequence of special case - "a": "a", - "/foo": "foo", - "foo/": "foo", - "/foo/": "foo", - "/foo/bar": "foo_bar", - "foo/bar/": "foo_bar", - "/foo/bar/": "foo_bar", - "/foo/{orgName}/Bar": "foo_orgname_bar", - "/foo/{org_name}/Bar": "foo_org_name_bar", - "/foo/{org__name}/Bar": "foo_org_name_bar", - "/foo/{org___name}/_Bar": "foo_org_name_bar", - "/foo.bar/baz.qux/": "foo_bar_baz_qux", - } { - if have := middleware.MakeLabelValue(input); want != have { - t.Errorf("%q: want %q, have %q", input, want, have) - } - } -} diff --git a/middleware/route_injector.go b/middleware/route_injector.go new file mode 100644 index 000000000..825367c70 --- /dev/null +++ b/middleware/route_injector.go @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package middleware + +import ( + "context" + "net/http" + "regexp" + "strings" + + "github.com/gorilla/mux" +) + +// RouteInjector is a middleware that injects the route name for the current request into the request context. +// +// The route name can be retrieved by calling ExtractRouteName. +type RouteInjector struct { + RouteMatcher RouteMatcher +} + +func (i RouteInjector) Wrap(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + routeName := getRouteName(i.RouteMatcher, r) + handler.ServeHTTP(w, WithRouteName(r, routeName)) + }) +} + +// WithRouteName annotates r's context with the provided route name. +// +// The provided value must be suitable to use as a Prometheus label value. +// +// This method should generally only be used in tests: in production code, use RouteInjector instead. +func WithRouteName(r *http.Request, routeName string) *http.Request { + ctx := context.WithValue(r.Context(), contextKeyRouteName, routeName) + return r.WithContext(ctx) +} + +// ExtractRouteName returns the route name associated with this request. +// This is the same route name used for trace and metric names, and is already suitable for use as a Prometheus label +// value. +func ExtractRouteName(r *http.Request) string { + routeName, ok := r.Context().Value(contextKeyRouteName).(string) + if !ok { + return "" + } + + return routeName +} + +func getRouteName(routeMatcher RouteMatcher, r *http.Request) string { + var routeMatch mux.RouteMatch + if routeMatcher == nil || !routeMatcher.Match(r, &routeMatch) { + return "" + } + + if routeMatch.MatchErr == mux.ErrNotFound { + return "notfound" + } + + if routeMatch.Route == nil { + return "" + } + + if name := routeMatch.Route.GetName(); name != "" { + return name + } + + tmpl, err := routeMatch.Route.GetPathTemplate() + if err == nil { + return MakeLabelValue(tmpl) + } + + return "" +} + +var invalidChars = regexp.MustCompile(`[^a-zA-Z0-9]+`) + +// MakeLabelValue converts a Gorilla mux path to a string suitable for use in +// a Prometheus label value. +func MakeLabelValue(path string) string { + // Convert non-alnums to underscores. + result := invalidChars.ReplaceAllString(path, "_") + + // Trim leading and trailing underscores. + result = strings.Trim(result, "_") + + // Make it all lowercase + result = strings.ToLower(result) + + // Special case. + if result == "" { + result = "root" + } + return result +} diff --git a/middleware/route_injector_test.go b/middleware/route_injector_test.go new file mode 100644 index 000000000..dea302f51 --- /dev/null +++ b/middleware/route_injector_test.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Provenance-includes-location: https://github.com/weaveworks/common/blob/main/middleware/instrument_test.go +// Provenance-includes-license: Apache-2.0 +// Provenance-includes-copyright: Weaveworks Ltd. + +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" + "github.com/stretchr/testify/require" +) + +func TestRouteInjector(t *testing.T) { + testCases := map[string]string{ + "/": "root", + "/foo/bar/blah": "foo_bar_blah", + "/templated/name-1/thing": "templated_name_thing", + "/named": "my-named-route", + "/does-not-exist": "notfound", + } + + for url, expectedRouteName := range testCases { + t.Run(url, func(t *testing.T) { + actualRouteName := "" + + handler := func(_ http.ResponseWriter, r *http.Request) { + actualRouteName = ExtractRouteName(r) + } + + router := mux.NewRouter() + router.HandleFunc("/", handler) + router.HandleFunc("/foo/bar/blah", handler) + router.HandleFunc("/templated/{name}/thing", handler) + router.HandleFunc("/named", handler).Name("my-named-route") + router.NotFoundHandler = http.HandlerFunc(handler) + + endpoint := RouteInjector{router}.Wrap(router) + endpoint.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, url, nil)) + + require.Equal(t, expectedRouteName, actualRouteName) + }) + } + +} + +func TestMakeLabelValue(t *testing.T) { + for input, want := range map[string]string{ + "/": "root", // special case + "//": "root", // unintended consequence of special case + "a": "a", + "/foo": "foo", + "foo/": "foo", + "/foo/": "foo", + "/foo/bar": "foo_bar", + "foo/bar/": "foo_bar", + "/foo/bar/": "foo_bar", + "/foo/{orgName}/Bar": "foo_orgname_bar", + "/foo/{org_name}/Bar": "foo_org_name_bar", + "/foo/{org__name}/Bar": "foo_org_name_bar", + "/foo/{org___name}/_Bar": "foo_org_name_bar", + "/foo.bar/baz.qux/": "foo_bar_baz_qux", + } { + if have := MakeLabelValue(input); want != have { + t.Errorf("%q: want %q, have %q", input, want, have) + } + } +} diff --git a/server/server.go b/server/server.go index c739228dd..a23eead38 100644 --- a/server/server.go +++ b/server/server.go @@ -517,13 +517,14 @@ func BuildHTTPMiddleware(cfg Config, router *mux.Router, metrics *Metrics, logge defaultLogMiddleware.DisableRequestSuccessLog = cfg.DisableRequestSuccessLog defaultHTTPMiddleware := []middleware.Interface{ - middleware.Tracer{ + middleware.RouteInjector{ RouteMatcher: router, - SourceIPs: sourceIPs, + }, + middleware.Tracer{ + SourceIPs: sourceIPs, }, defaultLogMiddleware, middleware.Instrument{ - RouteMatcher: router, Duration: metrics.RequestDuration, PerTenantDuration: metrics.PerTenantRequestDuration, PerTenantCallback: cfg.PerTenantDurationInstrumentation, From 1b878b277957bdc20493b1de012454f003c53b58 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Mon, 27 May 2024 16:52:41 +1000 Subject: [PATCH 2/3] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d74fba20..a3fa0e03f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -209,6 +209,7 @@ * `operation_duration_seconds` * [ENHANCEMENT] Add `outcome` label to `gate_duration_seconds` metric. Possible values are `rejected_canceled`, `rejected_deadline_exceeded`, `rejected_other`, and `permitted`. #512 * [ENHANCEMENT] Expose `InstancesWithTokensCount` and `InstancesWithTokensInZoneCount` in `ring.ReadRing` interface. #516 +* [ENHANCEMENT] Middleware: determine route name in a single place, and add `middleware.ExtractRouteName()` method to allow consuming applications to retrieve the route name. #527 * [BUGFIX] spanlogger: Support multiple tenant IDs. #59 * [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85 * [BUGFIX] Ring: `ring_member_ownership_percent` and `ring_tokens_owned` metrics are not updated on scale down. #109 From 3996871f40906c465d5a08e6174ce1471df06405 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Tue, 28 May 2024 11:23:35 +1000 Subject: [PATCH 3/3] Address PR feedback --- middleware/http_tracing.go | 2 +- middleware/instrument.go | 2 +- middleware/route_injector.go | 8 +++++--- middleware/route_injector_test.go | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/middleware/http_tracing.go b/middleware/http_tracing.go index b2c34d114..d75535ebe 100644 --- a/middleware/http_tracing.go +++ b/middleware/http_tracing.go @@ -130,7 +130,7 @@ func HTTPGRPCTracingInterceptor(router *mux.Router) grpc.UnaryServerInterceptor } func httpOperationNameFunc(r *http.Request) string { - routeName := ExtractRouteName(r) + routeName := ExtractRouteName(r.Context()) return getOperationName(routeName, r) } diff --git a/middleware/instrument.go b/middleware/instrument.go index 098fc9a05..9813077ce 100644 --- a/middleware/instrument.go +++ b/middleware/instrument.go @@ -118,7 +118,7 @@ func (i Instrument) Wrap(next http.Handler) http.Handler { // We do all this as we do not wish to emit high cardinality labels to // prometheus. func (i Instrument) getRouteName(r *http.Request) string { - route := ExtractRouteName(r) + route := ExtractRouteName(r.Context()) if route == "" { route = "other" } diff --git a/middleware/route_injector.go b/middleware/route_injector.go index 825367c70..7b275f74f 100644 --- a/middleware/route_injector.go +++ b/middleware/route_injector.go @@ -35,11 +35,13 @@ func WithRouteName(r *http.Request, routeName string) *http.Request { return r.WithContext(ctx) } -// ExtractRouteName returns the route name associated with this request. +// ExtractRouteName returns the route name associated with this request that was previously injected by the +// RouteInjector middleware or WithRouteName. +// // This is the same route name used for trace and metric names, and is already suitable for use as a Prometheus label // value. -func ExtractRouteName(r *http.Request) string { - routeName, ok := r.Context().Value(contextKeyRouteName).(string) +func ExtractRouteName(ctx context.Context) string { + routeName, ok := ctx.Value(contextKeyRouteName).(string) if !ok { return "" } diff --git a/middleware/route_injector_test.go b/middleware/route_injector_test.go index dea302f51..10d294879 100644 --- a/middleware/route_injector_test.go +++ b/middleware/route_injector_test.go @@ -28,7 +28,7 @@ func TestRouteInjector(t *testing.T) { actualRouteName := "" handler := func(_ http.ResponseWriter, r *http.Request) { - actualRouteName = ExtractRouteName(r) + actualRouteName = ExtractRouteName(r.Context()) } router := mux.NewRouter()