From 918f1d943ae00d6714db7877cb0365cba97f7577 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Wed, 2 Feb 2022 19:07:37 +0100 Subject: [PATCH] Instrumentation: Fix HTTP request instrumentation of authentication failures (#44234) (#44793) Moves the request tracing middleware earlier in the chain, just after the tracing middleware and before the log middleware. With these changes we'll be able to track authentication/authorization status failures that currently exits early and don't execute the request tracing middleware. In addition, there might be some other routes now being tracked with this that we didn't do before. Fixes #39590 (cherry picked from commit 0092d107649c2dfac03e3ab7411f1067a98ddad3) Co-authored-by: Marcus Efraimsson --- pkg/api/http_server.go | 1 + pkg/api/routing/route_register.go | 5 +- pkg/middleware/request_metrics.go | 86 +++++++++++++++++-------------- 3 files changed, 51 insertions(+), 41 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 5a33efa7706..70bf0c4c26f 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -432,6 +432,7 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() { m := hs.web m.Use(middleware.RequestTracing(hs.tracer)) + m.Use(middleware.RequestMetrics(hs.Features)) m.Use(middleware.Logger(hs.Cfg)) diff --git a/pkg/api/routing/route_register.go b/pkg/api/routing/route_register.go index 4c2a59eee25..f9751972951 100644 --- a/pkg/api/routing/route_register.go +++ b/pkg/api/routing/route_register.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/grafana/grafana/pkg/middleware" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/web" ) @@ -52,8 +51,8 @@ type RouteRegister interface { type RegisterNamedMiddleware func(name string) web.Handler -func ProvideRegister(features featuremgmt.FeatureToggles) *RouteRegisterImpl { - return NewRouteRegister(middleware.ProvideRouteOperationName, middleware.RequestMetrics(features)) +func ProvideRegister() *RouteRegisterImpl { + return NewRouteRegister(middleware.ProvideRouteOperationName) } // NewRouteRegister creates a new RouteRegister with all middlewares sent as params diff --git a/pkg/middleware/request_metrics.go b/pkg/middleware/request_metrics.go index 6429f95f318..d4923f233a6 100644 --- a/pkg/middleware/request_metrics.go +++ b/pkg/middleware/request_metrics.go @@ -45,50 +45,60 @@ func init() { } // RequestMetrics is a middleware handler that instruments the request. -func RequestMetrics(features featuremgmt.FeatureToggles) func(handler string) web.Handler { - return func(handler string) web.Handler { - return func(res http.ResponseWriter, req *http.Request, c *web.Context) { - rw := res.(web.ResponseWriter) - now := time.Now() - httpRequestsInFlight.Inc() - defer httpRequestsInFlight.Dec() +func RequestMetrics(features featuremgmt.FeatureToggles) 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 == "/metrics" { c.Next() + return + } - status := rw.Status() + rw := res.(web.ResponseWriter) + now := time.Now() + httpRequestsInFlight.Inc() + defer httpRequestsInFlight.Dec() + c.Map(c.Req) + c.Next() - code := sanitizeCode(status) - method := sanitizeMethod(req.Method) + handler := "unknown" - // enable histogram and disable summaries + counters for http requests. - if features.IsEnabled(featuremgmt.FlagDisableHttpRequestHistogram) { - duration := time.Since(now).Nanoseconds() / int64(time.Millisecond) - metrics.MHttpRequestTotal.WithLabelValues(handler, code, method).Inc() - metrics.MHttpRequestSummary.WithLabelValues(handler, code, method).Observe(float64(duration)) - } else { - // avoiding the sanitize functions for in the new instrumentation - // since they dont make much sense. We should remove them later. - histogram := httpRequestDurationHistogram. - WithLabelValues(handler, strconv.Itoa(rw.Status()), req.Method) - if traceID, ok := cw.ExtractSampledTraceID(c.Req.Context()); ok { - // Need to type-convert the Observer to an - // ExemplarObserver. This will always work for a - // HistogramVec. - histogram.(prometheus.ExemplarObserver).ObserveWithExemplar( - time.Since(now).Seconds(), prometheus.Labels{"traceID": traceID}, - ) - return - } - histogram.Observe(time.Since(now).Seconds()) + if routeOperation, exists := RouteOperationNameFromContext(c.Req.Context()); exists { + handler = routeOperation + } + + status := rw.Status() + + code := sanitizeCode(status) + method := sanitizeMethod(req.Method) + + // enable histogram and disable summaries + counters for http requests. + if features.IsEnabled(featuremgmt.FlagDisableHttpRequestHistogram) { + duration := time.Since(now).Nanoseconds() / int64(time.Millisecond) + metrics.MHttpRequestTotal.WithLabelValues(handler, code, method).Inc() + metrics.MHttpRequestSummary.WithLabelValues(handler, code, method).Observe(float64(duration)) + } else { + // avoiding the sanitize functions for in the new instrumentation + // since they dont make much sense. We should remove them later. + histogram := httpRequestDurationHistogram. + WithLabelValues(handler, code, req.Method) + if traceID, ok := cw.ExtractSampledTraceID(c.Req.Context()); ok { + // Need to type-convert the Observer to an + // ExemplarObserver. This will always work for a + // HistogramVec. + histogram.(prometheus.ExemplarObserver).ObserveWithExemplar( + time.Since(now).Seconds(), prometheus.Labels{"traceID": traceID}, + ) + return } + histogram.Observe(time.Since(now).Seconds()) + } - switch { - case strings.HasPrefix(req.RequestURI, "/api/datasources/proxy"): - countProxyRequests(status) - case strings.HasPrefix(req.RequestURI, "/api/"): - countApiRequests(status) - default: - countPageRequests(status) - } + switch { + case strings.HasPrefix(req.RequestURI, "/api/datasources/proxy"): + countProxyRequests(status) + case strings.HasPrefix(req.RequestURI, "/api/"): + countApiRequests(status) + default: + countPageRequests(status) } } }