From bf5b3a8517d9ea719950d206a20dff0fd5b3f6ca Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Fri, 11 Apr 2025 12:01:52 +0000 Subject: [PATCH 1/7] added chi route extraction and labels --- coderd/httpmw/prometheus.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index b96be84e879e3..532dccdb7519a 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -22,18 +22,18 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler Name: "requests_processed_total", Help: "The total number of processed API requests", }, []string{"code", "method", "path"}) - requestsConcurrent := factory.NewGauge(prometheus.GaugeOpts{ + requestsConcurrent := factory.NewGaugeVec(prometheus.GaugeOpts{ Namespace: "coderd", Subsystem: "api", Name: "concurrent_requests", Help: "The number of concurrent API requests.", - }) - websocketsConcurrent := factory.NewGauge(prometheus.GaugeOpts{ + }, []string{"path"}) + websocketsConcurrent := factory.NewGaugeVec(prometheus.GaugeOpts{ Namespace: "coderd", Subsystem: "api", Name: "concurrent_websockets", Help: "The total number of concurrent API websockets.", - }) + }, []string{"path"}) websocketsDist := factory.NewHistogramVec(prometheus.HistogramOpts{ Namespace: "coderd", Subsystem: "api", @@ -69,19 +69,21 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler panic("dev error: http.ResponseWriter is not *tracing.StatusWriter") } + path := rctx.RoutePattern() + var ( dist *prometheus.HistogramVec distOpts []string ) // We want to count WebSockets separately. if httpapi.IsWebsocketUpgrade(r) { - websocketsConcurrent.Inc() - defer websocketsConcurrent.Dec() + websocketsConcurrent.WithLabelValues(path).Inc() + defer websocketsConcurrent.WithLabelValues(path).Dec() dist = websocketsDist } else { - requestsConcurrent.Inc() - defer requestsConcurrent.Dec() + requestsConcurrent.WithLabelValues(path).Inc() + defer requestsConcurrent.WithLabelValues(path).Dec() dist = requestsDist distOpts = []string{method} @@ -89,7 +91,6 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler next.ServeHTTP(w, r) - path := rctx.RoutePattern() distOpts = append(distOpts, path) statusStr := strconv.Itoa(sw.Status) From 3ff76bb26313611cf7b5ee417d3c2fc131f22bcc Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 14 Apr 2025 10:03:50 +0000 Subject: [PATCH 2/7] added path matching to prometheus middleware --- coderd/httpmw/prometheus.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index 532dccdb7519a..393a02f4691a4 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -61,7 +61,7 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler var ( start = time.Now() method = r.Method - rctx = chi.RouteContext(r.Context()) + // rctx = chi.RouteContext(r.Context()) ) sw, ok := w.(*tracing.StatusWriter) @@ -69,7 +69,7 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler panic("dev error: http.ResponseWriter is not *tracing.StatusWriter") } - path := rctx.RoutePattern() + path := getRoutePattern(r) var ( dist *prometheus.HistogramVec @@ -99,3 +99,26 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler }) } } + +func getRoutePattern(r *http.Request) string { + rctx := chi.RouteContext(r.Context()) + if pattern := rctx.RoutePattern(); pattern != "" { + // Pattern is already available + return pattern + } + + routePath := r.URL.Path + if r.URL.RawPath != "" { + routePath = r.URL.RawPath + } + + tctx := chi.NewRouteContext() + if !rctx.Routes.Match(tctx, r.Method, routePath) { + // No matching pattern, so just return an empty string. + // It is done to avoid returning a static path for frontend requests. + return "" + } + + // tctx has the updated pattern, since Match mutates it + return tctx.RoutePattern() +} From d9f3dfa32e307c37e73e7fa5a2fa8320e51727a5 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 14 Apr 2025 10:20:01 +0000 Subject: [PATCH 3/7] added method to requests concurrent --- coderd/httpmw/prometheus.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index 393a02f4691a4..9636dcae5b8ba 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -27,7 +27,7 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler Subsystem: "api", Name: "concurrent_requests", Help: "The number of concurrent API requests.", - }, []string{"path"}) + }, []string{"method", "path"}) websocketsConcurrent := factory.NewGaugeVec(prometheus.GaugeOpts{ Namespace: "coderd", Subsystem: "api", @@ -82,8 +82,8 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler dist = websocketsDist } else { - requestsConcurrent.WithLabelValues(path).Inc() - defer requestsConcurrent.WithLabelValues(path).Dec() + requestsConcurrent.WithLabelValues(method, path).Inc() + defer requestsConcurrent.WithLabelValues(method, path).Dec() dist = requestsDist distOpts = []string{method} From b3b5b75eed6eaa2107e8b9d3e5ff5021ee318535 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 14 Apr 2025 12:14:48 +0000 Subject: [PATCH 4/7] added unit tests for the prometheus middleware --- coderd/httpmw/prometheus.go | 7 ++- coderd/httpmw/prometheus_test.go | 90 ++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index 9636dcae5b8ba..9cacefd2d5cc2 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -102,6 +102,10 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler func getRoutePattern(r *http.Request) string { rctx := chi.RouteContext(r.Context()) + if rctx == nil { + return "" + } + if pattern := rctx.RoutePattern(); pattern != "" { // Pattern is already available return pattern @@ -113,7 +117,8 @@ func getRoutePattern(r *http.Request) string { } tctx := chi.NewRouteContext() - if !rctx.Routes.Match(tctx, r.Method, routePath) { + routes := rctx.Routes + if routes != nil && routes.Match(tctx, r.Method, routePath) { // No matching pattern, so just return an empty string. // It is done to avoid returning a static path for frontend requests. return "" diff --git a/coderd/httpmw/prometheus_test.go b/coderd/httpmw/prometheus_test.go index a51eea5d00312..849fcb7e00420 100644 --- a/coderd/httpmw/prometheus_test.go +++ b/coderd/httpmw/prometheus_test.go @@ -8,10 +8,14 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + cm "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/tracing" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" ) func TestPrometheus(t *testing.T) { @@ -30,3 +34,89 @@ func TestPrometheus(t *testing.T) { require.Greater(t, len(metrics), 0) }) } + +func TestPrometheus_Concurrent(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + reg := prometheus.NewRegistry() + promMW := httpmw.Prometheus(reg) + + // Create a test handler to simulate a WebSocket connection + testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + conn, err := websocket.Accept(rw, r, nil) + if !assert.NoError(t, err, "failed to accept websocket") { + return + } + defer conn.Close(websocket.StatusGoingAway, "") + }) + + wrappedHandler := promMW(testHandler) + + r := chi.NewRouter() + r.Use(tracing.StatusWriterMiddleware, promMW) + r.Get("/api/v2/build/{build}/logs", func(rw http.ResponseWriter, r *http.Request) { + wrappedHandler.ServeHTTP(rw, r) + }) + + srv := httptest.NewServer(r) + defer srv.Close() + // nolint: bodyclose + conn, _, err := websocket.Dial(ctx, srv.URL+"/api/v2/build/1/logs", nil) + require.NoError(t, err, "failed to dial WebSocket") + defer conn.Close(websocket.StatusNormalClosure, "") + + metrics, err := reg.Gather() + require.NoError(t, err) + require.Greater(t, len(metrics), 0) + metricLabels := getMetricLabels(metrics) + + concurrentWebsockets, ok := metricLabels["coderd_api_concurrent_websockets"] + require.True(t, ok, "coderd_api_concurrent_websockets metric not found") + require.Equal(t, "/api/v2/build/{build}/logs", concurrentWebsockets["path"]) +} + +func TestGetRoutePattern_UserRoute(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + promMW := httpmw.Prometheus(reg) + + r := chi.NewRouter() + r.With(promMW).Get("/api/v2/users/{user}", func(w http.ResponseWriter, r *http.Request) {}) + + req := httptest.NewRequest("GET", "/api/v2/users/john", nil) + + sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} + + r.ServeHTTP(sw, req) + + metrics, err := reg.Gather() + require.NoError(t, err) + require.Greater(t, len(metrics), 0) + metricLabels := getMetricLabels(metrics) + + reqProcessed, ok := metricLabels["coderd_api_requests_processed_total"] + require.True(t, ok, "coderd_api_requests_processed_total metric not found") + require.Equal(t, "/api/v2/users/{user}", reqProcessed["path"]) + require.Equal(t, "GET", reqProcessed["method"]) + + concurrentRequests, ok := metricLabels["coderd_api_concurrent_requests"] + require.True(t, ok, "coderd_api_concurrent_requests metric not found") + require.Equal(t, "/api/v2/users/{user}", concurrentRequests["path"]) + require.Equal(t, "GET", concurrentRequests["method"]) +} + +func getMetricLabels(metrics []*cm.MetricFamily) map[string]map[string]string { + metricLabels := map[string]map[string]string{} + for _, metricFamily := range metrics { + metricName := metricFamily.GetName() + metricLabels[metricName] = map[string]string{} + for _, metric := range metricFamily.GetMetric() { + for _, labelPair := range metric.GetLabel() { + metricLabels[metricName][labelPair.GetName()] = labelPair.GetValue() + } + } + } + return metricLabels +} From 4d95f4eeedf47e5db79a56ae0c7ff8a4e2a993b2 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 15 Apr 2025 16:46:03 +0000 Subject: [PATCH 5/7] unified testing code style --- coderd/httpmw/prometheus_test.go | 117 ++++++++++++++++--------------- 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/coderd/httpmw/prometheus_test.go b/coderd/httpmw/prometheus_test.go index 849fcb7e00420..d40558f5ca5e7 100644 --- a/coderd/httpmw/prometheus_test.go +++ b/coderd/httpmw/prometheus_test.go @@ -20,6 +20,7 @@ import ( func TestPrometheus(t *testing.T) { t.Parallel() + t.Run("All", func(t *testing.T) { t.Parallel() req := httptest.NewRequest("GET", "/", nil) @@ -33,78 +34,78 @@ func TestPrometheus(t *testing.T) { require.NoError(t, err) require.Greater(t, len(metrics), 0) }) -} -func TestPrometheus_Concurrent(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + t.Run("Concurrent", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - reg := prometheus.NewRegistry() - promMW := httpmw.Prometheus(reg) + reg := prometheus.NewRegistry() + promMW := httpmw.Prometheus(reg) - // Create a test handler to simulate a WebSocket connection - testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - conn, err := websocket.Accept(rw, r, nil) - if !assert.NoError(t, err, "failed to accept websocket") { - return - } - defer conn.Close(websocket.StatusGoingAway, "") - }) + // Create a test handler to simulate a WebSocket connection + testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + conn, err := websocket.Accept(rw, r, nil) + if !assert.NoError(t, err, "failed to accept websocket") { + return + } + defer conn.Close(websocket.StatusGoingAway, "") + }) - wrappedHandler := promMW(testHandler) + wrappedHandler := promMW(testHandler) - r := chi.NewRouter() - r.Use(tracing.StatusWriterMiddleware, promMW) - r.Get("/api/v2/build/{build}/logs", func(rw http.ResponseWriter, r *http.Request) { - wrappedHandler.ServeHTTP(rw, r) - }) + r := chi.NewRouter() + r.Use(tracing.StatusWriterMiddleware, promMW) + r.Get("/api/v2/build/{build}/logs", func(rw http.ResponseWriter, r *http.Request) { + wrappedHandler.ServeHTTP(rw, r) + }) - srv := httptest.NewServer(r) - defer srv.Close() - // nolint: bodyclose - conn, _, err := websocket.Dial(ctx, srv.URL+"/api/v2/build/1/logs", nil) - require.NoError(t, err, "failed to dial WebSocket") - defer conn.Close(websocket.StatusNormalClosure, "") - - metrics, err := reg.Gather() - require.NoError(t, err) - require.Greater(t, len(metrics), 0) - metricLabels := getMetricLabels(metrics) - - concurrentWebsockets, ok := metricLabels["coderd_api_concurrent_websockets"] - require.True(t, ok, "coderd_api_concurrent_websockets metric not found") - require.Equal(t, "/api/v2/build/{build}/logs", concurrentWebsockets["path"]) -} + srv := httptest.NewServer(r) + defer srv.Close() + // nolint: bodyclose + conn, _, err := websocket.Dial(ctx, srv.URL+"/api/v2/build/1/logs", nil) + require.NoError(t, err, "failed to dial WebSocket") + defer conn.Close(websocket.StatusNormalClosure, "") -func TestGetRoutePattern_UserRoute(t *testing.T) { - t.Parallel() - reg := prometheus.NewRegistry() - promMW := httpmw.Prometheus(reg) + metrics, err := reg.Gather() + require.NoError(t, err) + require.Greater(t, len(metrics), 0) + metricLabels := getMetricLabels(metrics) - r := chi.NewRouter() - r.With(promMW).Get("/api/v2/users/{user}", func(w http.ResponseWriter, r *http.Request) {}) + concurrentWebsockets, ok := metricLabels["coderd_api_concurrent_websockets"] + require.True(t, ok, "coderd_api_concurrent_websockets metric not found") + require.Equal(t, "/api/v2/build/{build}/logs", concurrentWebsockets["path"]) + }) - req := httptest.NewRequest("GET", "/api/v2/users/john", nil) + t.Run("UserRoute", func(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + promMW := httpmw.Prometheus(reg) - sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} + r := chi.NewRouter() + r.With(promMW).Get("/api/v2/users/{user}", func(w http.ResponseWriter, r *http.Request) {}) - r.ServeHTTP(sw, req) + req := httptest.NewRequest("GET", "/api/v2/users/john", nil) - metrics, err := reg.Gather() - require.NoError(t, err) - require.Greater(t, len(metrics), 0) - metricLabels := getMetricLabels(metrics) + sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} - reqProcessed, ok := metricLabels["coderd_api_requests_processed_total"] - require.True(t, ok, "coderd_api_requests_processed_total metric not found") - require.Equal(t, "/api/v2/users/{user}", reqProcessed["path"]) - require.Equal(t, "GET", reqProcessed["method"]) + r.ServeHTTP(sw, req) - concurrentRequests, ok := metricLabels["coderd_api_concurrent_requests"] - require.True(t, ok, "coderd_api_concurrent_requests metric not found") - require.Equal(t, "/api/v2/users/{user}", concurrentRequests["path"]) - require.Equal(t, "GET", concurrentRequests["method"]) + metrics, err := reg.Gather() + require.NoError(t, err) + require.Greater(t, len(metrics), 0) + metricLabels := getMetricLabels(metrics) + + reqProcessed, ok := metricLabels["coderd_api_requests_processed_total"] + require.True(t, ok, "coderd_api_requests_processed_total metric not found") + require.Equal(t, "/api/v2/users/{user}", reqProcessed["path"]) + require.Equal(t, "GET", reqProcessed["method"]) + + concurrentRequests, ok := metricLabels["coderd_api_concurrent_requests"] + require.True(t, ok, "coderd_api_concurrent_requests metric not found") + require.Equal(t, "/api/v2/users/{user}", concurrentRequests["path"]) + require.Equal(t, "GET", concurrentRequests["method"]) + }) } func getMetricLabels(metrics []*cm.MetricFamily) map[string]map[string]string { From 2cb3796e00b590e1c699675ba163895827a8d871 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 16 Apr 2025 07:52:20 +0000 Subject: [PATCH 6/7] PR feedback --- coderd/httpmw/prometheus.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index 9cacefd2d5cc2..04cbd98f8ba81 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -3,6 +3,7 @@ package httpmw import ( "net/http" "strconv" + "strings" "time" "github.com/go-chi/chi/v5" @@ -61,7 +62,6 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler var ( start = time.Now() method = r.Method - // rctx = chi.RouteContext(r.Context()) ) sw, ok := w.(*tracing.StatusWriter) @@ -69,12 +69,12 @@ func Prometheus(register prometheus.Registerer) func(http.Handler) http.Handler panic("dev error: http.ResponseWriter is not *tracing.StatusWriter") } - path := getRoutePattern(r) - var ( dist *prometheus.HistogramVec distOpts []string + path = getRoutePattern(r) ) + // We want to count WebSockets separately. if httpapi.IsWebsocketUpgrade(r) { websocketsConcurrent.WithLabelValues(path).Inc() @@ -119,9 +119,12 @@ func getRoutePattern(r *http.Request) string { tctx := chi.NewRouteContext() routes := rctx.Routes if routes != nil && routes.Match(tctx, r.Method, routePath) { - // No matching pattern, so just return an empty string. - // It is done to avoid returning a static path for frontend requests. - return "" + // No matching pattern. /api/* requests will be matched as "UNKNOWN" + // All other ones will be matched as "STATIC". + if strings.HasPrefix(routePath, "/api/") { + return "UNKNOWN" + } + return "STATIC" } // tctx has the updated pattern, since Match mutates it From b68853a453441996bda692e5167342ba823501a2 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 16 Apr 2025 08:19:06 +0000 Subject: [PATCH 7/7] fixed a bug with path matching --- coderd/httpmw/prometheus.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index 04cbd98f8ba81..8b7b33381c74d 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -118,7 +118,7 @@ func getRoutePattern(r *http.Request) string { tctx := chi.NewRouteContext() routes := rctx.Routes - if routes != nil && routes.Match(tctx, r.Method, routePath) { + if routes != nil && !routes.Match(tctx, r.Method, routePath) { // No matching pattern. /api/* requests will be matched as "UNKNOWN" // All other ones will be matched as "STATIC". if strings.HasPrefix(routePath, "/api/") {