From b8b6d0e1a1b31bdddb9ebf0e670e01101908b1b9 Mon Sep 17 00:00:00 2001 From: Carl Bergquist Date: Tue, 14 Jun 2022 07:58:20 +0200 Subject: [PATCH] Instrumentation: Define handlers for requests that are not handled with named handlers (#50613) Signed-off-by: bergquist Co-authored-by: Marcus Efraimsson Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> --- pkg/middleware/request_metrics.go | 17 ++++++------- pkg/middleware/request_test.go | 40 +++++++++++++++++++++++++++++++ pkg/middleware/request_tracing.go | 31 ++++++++++++++++++++---- 3 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 pkg/middleware/request_test.go diff --git a/pkg/middleware/request_metrics.go b/pkg/middleware/request_metrics.go index fb6c7a57117..8c8d193e12a 100644 --- a/pkg/middleware/request_metrics.go +++ b/pkg/middleware/request_metrics.go @@ -50,11 +50,6 @@ func RequestMetrics(features featuremgmt.FeatureToggles) web.Handler { log := log.New("middleware.request-metrics") return func(res http.ResponseWriter, req *http.Request, c *web.Context) { - if strings.HasPrefix(c.Req.URL.Path, "/public/") || c.Req.URL.Path == "robots.txt" || c.Req.URL.Path == "/metrics" { - c.Next() - return - } - rw := res.(web.ResponseWriter) now := time.Now() httpRequestsInFlight.Inc() @@ -65,11 +60,17 @@ func RequestMetrics(features featuremgmt.FeatureToggles) web.Handler { code := sanitizeCode(status) handler := "unknown" - if routeOperation, exists := RouteOperationNameFromContext(c.Req.Context()); exists { + if routeOperation, exists := routeOperationName(c.Req); exists { handler = routeOperation } else { - if features.IsEnabled(featuremgmt.FlagLogRequestsInstrumentedAsUnknown) { - log.Warn("request instrumented as unknown", "path", c.Req.URL.Path, "status_code", status) + // if grafana does not recognize the handler and returns 404 we should register it as `notfound` + if status == http.StatusNotFound { + handler = "notfound" + } else { + // log requests where we could not identify handler so we can register them. + if features.IsEnabled(featuremgmt.FlagLogRequestsInstrumentedAsUnknown) { + log.Warn("request instrumented as unknown", "path", c.Req.URL.Path, "status_code", status) + } } } diff --git a/pkg/middleware/request_test.go b/pkg/middleware/request_test.go new file mode 100644 index 00000000000..16b41b9f231 --- /dev/null +++ b/pkg/middleware/request_test.go @@ -0,0 +1,40 @@ +package middleware + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCanGetRouteNameFromContext(t *testing.T) { + tcs := []struct { + namedHandler string + path string + + expected string + }{ + {namedHandler: "", path: "/public/img/apple-touch-icon.png", expected: "public-assets"}, + {namedHandler: "", path: "/favicon.ico", expected: "public-assets"}, + {namedHandler: "", path: "/robots.txt", expected: "/robots.txt"}, + {namedHandler: "", path: "/debug/pprof/heap", expected: "/debug/pprof-handlers"}, + {namedHandler: "", path: "/debug/pprof/allocs", expected: "/debug/pprof-handlers"}, + {namedHandler: "", path: "/debug/pprof/threadcreate", expected: "/debug/pprof-handlers"}, + {namedHandler: "/api/dashboard/:uid", path: "/api/dashboard/ddfgeasdfr", expected: "/api/dashboard/:uid"}, + {namedHandler: "", path: "/metrics", expected: "/metrics"}, + } + + for _, tc := range tcs { + req, err := http.NewRequest(http.MethodPost, "https://ops.grafana.net"+tc.path, nil) + + if tc.namedHandler != "" { + req = req.WithContext(context.WithValue(context.Background(), routeOperationNameKey, tc.namedHandler)) + } + + assert.NoError(t, err) + + handler, _ := routeOperationName(req) + assert.Equal(t, tc.expected, handler) + } +} diff --git a/pkg/middleware/request_tracing.go b/pkg/middleware/request_tracing.go index e477749ce9a..dcfce62fabe 100644 --- a/pkg/middleware/request_tracing.go +++ b/pkg/middleware/request_tracing.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "regexp" "strconv" "strings" @@ -31,20 +32,40 @@ func ProvideRouteOperationName(name string) web.Handler { } } -// RouteOperationNameFromContext receives the route operation name from context, if set. -func RouteOperationNameFromContext(ctx context.Context) (string, bool) { - if val := ctx.Value(routeOperationNameKey); val != nil { +var unnamedHandlers = []struct { + pathPattern *regexp.Regexp + handler string +}{ + {handler: "public-assets", pathPattern: regexp.MustCompile("^/favicon.ico")}, + {handler: "public-assets", pathPattern: regexp.MustCompile("^/public/")}, + {handler: "/metrics", pathPattern: regexp.MustCompile("^/metrics")}, + {handler: "/healthz", pathPattern: regexp.MustCompile("^/healthz")}, + {handler: "/robots.txt", pathPattern: regexp.MustCompile("^/robots.txt$")}, + // bundle all pprof endpoints under the same handler name + {handler: "/debug/pprof-handlers", pathPattern: regexp.MustCompile("^/debug/pprof")}, +} + +// routeOperationName receives the route operation name from context, if set. +func routeOperationName(req *http.Request) (string, bool) { + if val := req.Context().Value(routeOperationNameKey); val != nil { op, ok := val.(string) return op, ok } + for _, hp := range unnamedHandlers { + if hp.pathPattern.Match([]byte(req.URL.Path)) { + return hp.handler, true + } + } + return "", false } func RequestTracing(tracer tracing.Tracer) web.Handler { return func(res http.ResponseWriter, req *http.Request, c *web.Context) { if strings.HasPrefix(c.Req.URL.Path, "/public/") || - c.Req.URL.Path == "robots.txt" { + c.Req.URL.Path == "/robots.txt" || + c.Req.URL.Path == "/favicon.ico" { c.Next() return } @@ -59,7 +80,7 @@ func RequestTracing(tracer tracing.Tracer) web.Handler { // Only call span.Finish when a route operation name have been set, // meaning that not set the span would not be reported. - if routeOperation, exists := RouteOperationNameFromContext(c.Req.Context()); exists { + if routeOperation, exists := routeOperationName(c.Req); exists { defer span.End() span.SetName(fmt.Sprintf("HTTP %s %s", req.Method, routeOperation)) }