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 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..d75535ebe 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.Context()) + return getOperationName(routeName, r) } func getOperationName(routeName string, r *http.Request) string { diff --git a/middleware/instrument.go b/middleware/instrument.go index 6e821e4c0..9813077ce 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.Context()) 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..7b275f74f --- /dev/null +++ b/middleware/route_injector.go @@ -0,0 +1,97 @@ +// 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 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(ctx context.Context) string { + routeName, ok := ctx.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..10d294879 --- /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.Context()) + } + + 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,