From af6f5da200d5e05499931ebfc06c694b84f426b9 Mon Sep 17 00:00:00 2001 From: Vanshika Date: Tue, 23 Jun 2026 00:11:28 +0530 Subject: [PATCH 1/4] feat(picod): add Prometheus metrics endpoint Implement a /metrics Prometheus endpoint in PicoD to monitor command executions, request latency, and HTTP requests. - Add metrics for active executions and request outcomes. - Add Gin middleware for path/method/status metrics. - Exclude /metrics from auth and gzip compression. - Add unit tests. Signed-off-by: Vanshika --- go.mod | 2 +- pkg/picod/execute.go | 67 +++++++++++----- pkg/picod/metrics.go | 114 +++++++++++++++++++++++++++ pkg/picod/metrics_test.go | 157 ++++++++++++++++++++++++++++++++++++++ pkg/picod/server.go | 8 +- 5 files changed, 325 insertions(+), 23 deletions(-) create mode 100644 pkg/picod/metrics.go create mode 100644 pkg/picod/metrics_test.go diff --git a/go.mod b/go.mod index 9e73c5019..0cc13f3f9 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/gin-gonic/gin v1.10.0 github.com/golang-jwt/jwt/v5 v5.2.2 github.com/google/uuid v1.6.0 + github.com/prometheus/client_golang v1.23.2 github.com/redis/go-redis/v9 v9.17.1 github.com/stretchr/testify v1.11.1 github.com/valkey-io/valkey-go v1.0.69 @@ -75,7 +76,6 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pelletier/go-toml/v2 v2.2.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.67.1 // indirect github.com/prometheus/procfs v0.17.0 // indirect diff --git a/pkg/picod/execute.go b/pkg/picod/execute.go index d493f5fd6..2319a0e25 100644 --- a/pkg/picod/execute.go +++ b/pkg/picod/execute.go @@ -53,8 +53,12 @@ type ExecuteResponse struct { // ExecuteHandler handles command execution requests func (s *Server) ExecuteHandler(c *gin.Context) { + s.metrics.ActiveExecutions.Inc() + defer s.metrics.ActiveExecutions.Dec() + var req ExecuteRequest if err := c.ShouldBindJSON(&req); err != nil { + s.metrics.ExecuteRequestsTotal.WithLabelValues("invalid").Inc() c.JSON(http.StatusBadRequest, gin.H{ "error": err.Error(), "code": http.StatusBadRequest, @@ -63,6 +67,7 @@ func (s *Server) ExecuteHandler(c *gin.Context) { } if len(req.Command) == 0 { + s.metrics.ExecuteRequestsTotal.WithLabelValues("invalid").Inc() c.JSON(http.StatusBadRequest, gin.H{ "error": "command cannot be empty", "code": http.StatusBadRequest, @@ -76,6 +81,7 @@ func (s *Server) ExecuteHandler(c *gin.Context) { var err error timeoutDuration, err = time.ParseDuration(req.Timeout) if err != nil { + s.metrics.ExecuteRequestsTotal.WithLabelValues("invalid").Inc() c.JSON(http.StatusBadRequest, gin.H{ "error": fmt.Sprintf("Invalid timeout format: %v", err), "code": http.StatusBadRequest, @@ -92,28 +98,11 @@ func (s *Server) ExecuteHandler(c *gin.Context) { // Use the first element as the command and the rest as arguments cmd := exec.CommandContext(ctx, req.Command[0], req.Command[1:]...) //nolint:gosec // This is an agent designed to execute arbitrary commands - // Default working directory to workspace; override if the request specifies one. - cmd.Dir = s.workspaceDir - if req.WorkingDir != "" { - safeWorkingDir, err := s.sanitizePath(req.WorkingDir) - if err != nil { - c.JSON(http.StatusBadRequest, gin.H{ - "error": fmt.Sprintf("Invalid working directory: %v", err), - "code": http.StatusBadRequest, - }) - return - } - if _, statErr := os.Stat(safeWorkingDir); os.IsNotExist(statErr) { - if err := s.mkdirSafe(safeWorkingDir); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{ - "error": fmt.Sprintf("Failed to create working directory: %v", err), - "code": http.StatusInternalServerError, - }) - return - } - } - cmd.Dir = safeWorkingDir + workingDir, ok := s.prepareWorkingDir(c, req.WorkingDir) + if !ok { + return } + cmd.Dir = workingDir // Set environment variables if len(req.Env) > 0 { @@ -134,13 +123,21 @@ func (s *Server) ExecuteHandler(c *gin.Context) { endTime := time.Now() var exitCode int + var outcomeStatus string if errors.Is(ctx.Err(), context.DeadlineExceeded) { exitCode = TimeoutExitCode + outcomeStatus = "timeout" stderr.WriteString(fmt.Sprintf("Command timed out after %.0f seconds", timeoutDuration.Seconds())) } else if cmd.ProcessState != nil { exitCode = cmd.ProcessState.ExitCode() + if exitCode == 0 { + outcomeStatus = "success" + } else { + outcomeStatus = "error" + } } else { exitCode = 1 + outcomeStatus = "error" if stderr.Len() > 0 { stderr.WriteString("\n") } @@ -150,6 +147,8 @@ func (s *Server) ExecuteHandler(c *gin.Context) { } } + s.metrics.ExecuteRequestsTotal.WithLabelValues(outcomeStatus).Inc() + c.JSON(http.StatusOK, ExecuteResponse{ Stdout: stdout.String(), Stderr: stderr.String(), @@ -159,3 +158,29 @@ func (s *Server) ExecuteHandler(c *gin.Context) { EndTime: endTime, }) } + +func (s *Server) prepareWorkingDir(c *gin.Context, reqWorkingDir string) (string, bool) { + if reqWorkingDir == "" { + return s.workspaceDir, true + } + safeWorkingDir, err := s.sanitizePath(reqWorkingDir) + if err != nil { + s.metrics.ExecuteRequestsTotal.WithLabelValues("invalid").Inc() + c.JSON(http.StatusBadRequest, gin.H{ + "error": fmt.Sprintf("Invalid working directory: %v", err), + "code": http.StatusBadRequest, + }) + return "", false + } + if _, statErr := os.Stat(safeWorkingDir); os.IsNotExist(statErr) { + if err := s.mkdirSafe(safeWorkingDir); err != nil { + s.metrics.ExecuteRequestsTotal.WithLabelValues("error").Inc() + c.JSON(http.StatusInternalServerError, gin.H{ + "error": fmt.Sprintf("Failed to create working directory: %v", err), + "code": http.StatusInternalServerError, + }) + return "", false + } + } + return safeWorkingDir, true +} diff --git a/pkg/picod/metrics.go b/pkg/picod/metrics.go new file mode 100644 index 000000000..534303789 --- /dev/null +++ b/pkg/picod/metrics.go @@ -0,0 +1,114 @@ +/* +Copyright The Volcano Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package picod + +import ( + "strconv" + "time" + + "github.com/gin-gonic/gin" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" +) + +// Metrics holds the Prometheus collectors for PicoD. +type Metrics struct { + Registry *prometheus.Registry + ActiveExecutions prometheus.Gauge + ExecuteRequestsTotal *prometheus.CounterVec + HTTPRequestsTotal *prometheus.CounterVec + HTTPRequestDuration *prometheus.HistogramVec +} + +// NewMetrics creates and registers metrics collectors with a private registry. +func NewMetrics() *Metrics { + reg := prometheus.NewRegistry() + + activeExecutions := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "picod_active_executions", + Help: "Number of execute handler invocations currently in flight (including validation).", + }) + + executeRequestsTotal := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "picod_execute_requests_total", + Help: "Total number of execute requests, partitioned by status (success, error, timeout, invalid).", + }, + []string{"status"}, + ) + + httpRequestsTotal := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "picod_http_requests_total", + Help: "Total number of HTTP requests processed, partitioned by method, path, and status code.", + }, + []string{"method", "path", "status_code"}, + ) + + httpRequestDuration := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "picod_http_request_duration_seconds", + Help: "Latency of HTTP requests, partitioned by method and path.", + Buckets: prometheus.DefBuckets, + }, + []string{"method", "path"}, + ) + + reg.MustRegister(activeExecutions) + reg.MustRegister(executeRequestsTotal) + reg.MustRegister(httpRequestsTotal) + reg.MustRegister(httpRequestDuration) + + return &Metrics{ + Registry: reg, + ActiveExecutions: activeExecutions, + ExecuteRequestsTotal: executeRequestsTotal, + HTTPRequestsTotal: httpRequestsTotal, + HTTPRequestDuration: httpRequestDuration, + } +} + +// Handler returns an HTTP handler for the metrics registry. +func (m *Metrics) Handler() gin.HandlerFunc { + h := promhttp.HandlerFor(m.Registry, promhttp.HandlerOpts{}) + return func(c *gin.Context) { + h.ServeHTTP(c.Writer, c.Request) + } +} + +// Middleware returns a Gin middleware that records HTTP request metrics. +func (m *Metrics) Middleware() gin.HandlerFunc { + return func(c *gin.Context) { + path := c.FullPath() + if path == "" { + path = "unmatched" + } + // Do not record requests to /metrics or /health to avoid noise + if path == "/metrics" || path == "/health" { + c.Next() + return + } + + start := time.Now() + c.Next() + duration := time.Since(start).Seconds() + + status := strconv.Itoa(c.Writer.Status()) + m.HTTPRequestsTotal.WithLabelValues(c.Request.Method, path, status).Inc() + m.HTTPRequestDuration.WithLabelValues(c.Request.Method, path).Observe(duration) + } +} diff --git a/pkg/picod/metrics_test.go b/pkg/picod/metrics_test.go new file mode 100644 index 000000000..3482c8bea --- /dev/null +++ b/pkg/picod/metrics_test.go @@ -0,0 +1,157 @@ +/* +Copyright The Volcano Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package picod + +import ( + "bytes" + "encoding/json" + "net/http" + "os" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMetrics_Exposition(t *testing.T) { + routerPriv, routerPubStr := generateRSAKeys(t) + server, ts, tmpDir := setupTestServer(t, routerPubStr) + defer os.RemoveAll(tmpDir) + defer ts.Close() + defer os.Unsetenv(PublicKeyEnvVar) + + client := ts.Client() + + // 1. Check /metrics endpoint is reachable and returns HTTP 200 + resp, err := client.Get(ts.URL + "/metrics") + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + resp.Body.Close() + + // 2. Perform an execution request + execReq := ExecuteRequest{ + Command: []string{"echo", "test-metrics"}, + } + bodyBytes, err := json.Marshal(execReq) + require.NoError(t, err) + + claims := jwt.MapClaims{ + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour * 6).Unix(), + } + token := createToken(t, routerPriv, claims) + + req, err := http.NewRequest("POST", ts.URL+"/api/execute", bytes.NewBuffer(bodyBytes)) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + + resp, err = client.Do(req) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + resp.Body.Close() + + // 3. Programmatically verify the metrics from the registry + metricFamilies, err := server.metrics.Registry.Gather() + require.NoError(t, err) + + var foundActiveExecutions, foundExecuteRequests, foundHTTPRequests, foundHTTPRequestDuration bool + + for _, mf := range metricFamilies { + switch *mf.Name { + case "picod_active_executions": + foundActiveExecutions = true + verifyActiveExecutions(t, mf) + + case "picod_execute_requests_total": + foundExecuteRequests = true + verifyExecuteRequests(t, mf) + + case "picod_http_requests_total": + foundHTTPRequests = true + verifyHTTPRequests(t, mf) + + case "picod_http_request_duration_seconds": + foundHTTPRequestDuration = true + verifyHTTPRequestDuration(t, mf) + } + } + + assert.True(t, foundActiveExecutions, "picod_active_executions should be registered") + assert.True(t, foundExecuteRequests, "picod_execute_requests_total should be registered") + assert.True(t, foundHTTPRequests, "picod_http_requests_total should be registered") + assert.True(t, foundHTTPRequestDuration, "picod_http_request_duration_seconds should be registered") +} + +func verifyActiveExecutions(t *testing.T, mf *dto.MetricFamily) { + require.Len(t, mf.Metric, 1) + assert.Equal(t, 0.0, *mf.Metric[0].Gauge.Value) +} + +func verifyExecuteRequests(t *testing.T, mf *dto.MetricFamily) { + require.Len(t, mf.Metric, 1) + assert.Equal(t, 1.0, *mf.Metric[0].Counter.Value) + require.Len(t, mf.Metric[0].Label, 1) + assert.Equal(t, "status", *mf.Metric[0].Label[0].Name) + assert.Equal(t, "success", *mf.Metric[0].Label[0].Value) +} + +func verifyHTTPRequests(t *testing.T, mf *dto.MetricFamily) { + var foundExecute bool + for _, m := range mf.Metric { + var path, method, status string + for _, label := range m.Label { + switch *label.Name { + case "path": + path = *label.Value + case "method": + method = *label.Value + case "status_code": + status = *label.Value + } + } + if path == "/api/execute" && method == "POST" && status == "200" { + foundExecute = true + assert.Equal(t, 1.0, *m.Counter.Value) + } + } + assert.True(t, foundExecute, "should record POST /api/execute 200 metric") +} + +func verifyHTTPRequestDuration(t *testing.T, mf *dto.MetricFamily) { + var foundExecute bool + for _, m := range mf.Metric { + var path, method string + for _, label := range m.Label { + switch *label.Name { + case "path": + path = *label.Value + case "method": + method = *label.Value + } + } + if path == "/api/execute" && method == "POST" { + foundExecute = true + assert.Greater(t, *m.Histogram.SampleCount, uint64(0)) + assert.Greater(t, *m.Histogram.SampleSum, 0.0) + } + } + assert.True(t, foundExecute, "should record duration for POST /api/execute") +} diff --git a/pkg/picod/server.go b/pkg/picod/server.go index 2bae2ff4a..da27b75ed 100644 --- a/pkg/picod/server.go +++ b/pkg/picod/server.go @@ -46,6 +46,7 @@ type Server struct { authManager *AuthManager startTime time.Time workspaceDir string + metrics *Metrics } // NewServer creates a new PicoD server instance @@ -54,6 +55,7 @@ func NewServer(config Config) *Server { config: config, startTime: time.Now(), authManager: NewAuthManager(), + metrics: NewMetrics(), } // Initialize workspace directory @@ -80,8 +82,9 @@ func NewServer(config Config) *Server { engine.Use(gin.Logger()) // Request logging engine.Use(gin.Recovery()) // Crash recovery engine.Use(maxBodySizeMiddleware()) + engine.Use(s.metrics.Middleware()) engine.MaxMultipartMemory = MaxBodySize - engine.Use(gzip.Gzip(gzip.BestSpeed, gzip.WithExcludedPaths([]string{"/health"}))) // Response compression + engine.Use(gzip.Gzip(gzip.BestSpeed, gzip.WithExcludedPaths([]string{"/health", "/metrics"}))) // Response compression // Load public key from environment variable (required for JWT auth) if err := s.authManager.LoadPublicKeyFromEnv(); err != nil { @@ -101,6 +104,9 @@ func NewServer(config Config) *Server { // Health check (no authentication required) engine.GET("/health", s.HealthCheckHandler) + // Metrics endpoint (no authentication required) + engine.GET("/metrics", s.metrics.Handler()) + s.engine = engine return s } From 41d74f46aa2c0ac878c06706c4fd08003d3c0899 Mon Sep 17 00:00:00 2001 From: Vanshika Date: Thu, 2 Jul 2026 22:03:27 +0530 Subject: [PATCH 2/4] fix(picod): record metrics before body-size check, add regression tests Move s.metrics.Middleware() before maxBodySizeMiddleware() so requests rejected with 413 are captured in picod_http_requests_total and picod_http_request_duration_seconds. Add TestMetrics_OversizedRequest to verify a request exceeding MaxBodySize produces a 413 response and is recorded with the correct metric labels. Add TestMetrics_UnmatchedRoute to verify the path label remains "unmatched" for routes that do not match any handler, preventing high-cardinality label values from raw URL paths entering the registry. Signed-off-by: Vanshika --- go.mod | 2 +- pkg/picod/metrics_test.go | 101 +++++++++++++++++++++++++++++++++++--- pkg/picod/server.go | 2 +- 3 files changed, 95 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index 0cc13f3f9..066f0ae8f 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/golang-jwt/jwt/v5 v5.2.2 github.com/google/uuid v1.6.0 github.com/prometheus/client_golang v1.23.2 + github.com/prometheus/client_model v0.6.2 github.com/redis/go-redis/v9 v9.17.1 github.com/stretchr/testify v1.11.1 github.com/valkey-io/valkey-go v1.0.69 @@ -76,7 +77,6 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pelletier/go-toml/v2 v2.2.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.67.1 // indirect github.com/prometheus/procfs v0.17.0 // indirect github.com/spf13/pflag v1.0.10 // indirect diff --git a/pkg/picod/metrics_test.go b/pkg/picod/metrics_test.go index 3482c8bea..1ca2a9870 100644 --- a/pkg/picod/metrics_test.go +++ b/pkg/picod/metrics_test.go @@ -20,6 +20,7 @@ import ( "bytes" "encoding/json" "net/http" + "net/http/httptest" "os" "testing" "time" @@ -30,6 +31,13 @@ import ( "github.com/stretchr/testify/require" ) +const ( + metricHTTPRequestsTotal = "picod_http_requests_total" + labelPath = "path" + labelMethod = "method" + executeAPIPath = "/api/execute" +) + func TestMetrics_Exposition(t *testing.T) { routerPriv, routerPubStr := generateRSAKeys(t) server, ts, tmpDir := setupTestServer(t, routerPubStr) @@ -58,7 +66,7 @@ func TestMetrics_Exposition(t *testing.T) { } token := createToken(t, routerPriv, claims) - req, err := http.NewRequest("POST", ts.URL+"/api/execute", bytes.NewBuffer(bodyBytes)) + req, err := http.NewRequest(http.MethodPost, ts.URL+executeAPIPath, bytes.NewBuffer(bodyBytes)) require.NoError(t, err) req.Header.Set("Authorization", "Bearer "+token) req.Header.Set("Content-Type", "application/json") @@ -84,7 +92,7 @@ func TestMetrics_Exposition(t *testing.T) { foundExecuteRequests = true verifyExecuteRequests(t, mf) - case "picod_http_requests_total": + case metricHTTPRequestsTotal: foundHTTPRequests = true verifyHTTPRequests(t, mf) @@ -119,15 +127,15 @@ func verifyHTTPRequests(t *testing.T, mf *dto.MetricFamily) { var path, method, status string for _, label := range m.Label { switch *label.Name { - case "path": + case labelPath: path = *label.Value - case "method": + case labelMethod: method = *label.Value case "status_code": status = *label.Value } } - if path == "/api/execute" && method == "POST" && status == "200" { + if path == executeAPIPath && method == http.MethodPost && status == "200" { foundExecute = true assert.Equal(t, 1.0, *m.Counter.Value) } @@ -141,13 +149,13 @@ func verifyHTTPRequestDuration(t *testing.T, mf *dto.MetricFamily) { var path, method string for _, label := range m.Label { switch *label.Name { - case "path": + case labelPath: path = *label.Value - case "method": + case labelMethod: method = *label.Value } } - if path == "/api/execute" && method == "POST" { + if path == executeAPIPath && method == http.MethodPost { foundExecute = true assert.Greater(t, *m.Histogram.SampleCount, uint64(0)) assert.Greater(t, *m.Histogram.SampleSum, 0.0) @@ -155,3 +163,80 @@ func verifyHTTPRequestDuration(t *testing.T, mf *dto.MetricFamily) { } assert.True(t, foundExecute, "should record duration for POST /api/execute") } + +func TestMetrics_OversizedRequest(t *testing.T) { + _, routerPubStr := generateRSAKeys(t) + server, ts, tmpDir := setupTestServer(t, routerPubStr) + defer os.RemoveAll(tmpDir) + defer ts.Close() + defer os.Unsetenv(PublicKeyEnvVar) + + // Use ServeHTTP directly to set ContentLength without HTTP client validation. + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, executeAPIPath, nil) + r.ContentLength = int64(MaxBodySize) + 1 + r.Header.Set("Content-Type", "application/json") + server.engine.ServeHTTP(w, r) + assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code) + + metricFamilies, err := server.metrics.Registry.Gather() + require.NoError(t, err) + + var found413 bool + for _, mf := range metricFamilies { + if *mf.Name != metricHTTPRequestsTotal { + continue + } + for _, m := range mf.Metric { + var path, method, status string + for _, label := range m.Label { + switch *label.Name { + case labelPath: + path = *label.Value + case labelMethod: + method = *label.Value + case "status_code": + status = *label.Value + } + } + if path == executeAPIPath && method == http.MethodPost && status == "413" { + found413 = true + assert.Equal(t, 1.0, *m.Counter.Value) + } + } + } + assert.True(t, found413, "oversized request should be recorded with POST /api/execute status_code=413") +} + +func TestMetrics_UnmatchedRoute(t *testing.T) { + _, routerPubStr := generateRSAKeys(t) + server, ts, tmpDir := setupTestServer(t, routerPubStr) + defer os.RemoveAll(tmpDir) + defer ts.Close() + defer os.Unsetenv(PublicKeyEnvVar) + + client := ts.Client() + + resp, err := client.Get(ts.URL + "/does-not-exist") + require.NoError(t, err) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + resp.Body.Close() + + metricFamilies, err := server.metrics.Registry.Gather() + require.NoError(t, err) + + var foundUnmatched bool + for _, mf := range metricFamilies { + if *mf.Name != metricHTTPRequestsTotal { + continue + } + for _, m := range mf.Metric { + for _, label := range m.Label { + if *label.Name == labelPath && *label.Value == "unmatched" { + foundUnmatched = true + } + } + } + } + assert.True(t, foundUnmatched, "unmatched routes must use path label 'unmatched', not the raw URL path") +} diff --git a/pkg/picod/server.go b/pkg/picod/server.go index da27b75ed..d91fd91f6 100644 --- a/pkg/picod/server.go +++ b/pkg/picod/server.go @@ -81,8 +81,8 @@ func NewServer(config Config) *Server { // Global middleware engine.Use(gin.Logger()) // Request logging engine.Use(gin.Recovery()) // Crash recovery - engine.Use(maxBodySizeMiddleware()) engine.Use(s.metrics.Middleware()) + engine.Use(maxBodySizeMiddleware()) engine.MaxMultipartMemory = MaxBodySize engine.Use(gzip.Gzip(gzip.BestSpeed, gzip.WithExcludedPaths([]string{"/health", "/metrics"}))) // Response compression From d4822103f00c34373ea8c0bf7040093491f37d5a Mon Sep 17 00:00:00 2001 From: Vanshika Date: Wed, 8 Jul 2026 14:57:54 +0530 Subject: [PATCH 3/4] fix(picod): wrap gin.Recovery with metrics middleware Move s.metrics.Middleware() before gin.Recovery() so that panics caught by Recovery still have their 500 responses recorded in picod_http_requests_total and picod_http_request_duration_seconds. Signed-off-by: Vanshika --- pkg/picod/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/picod/server.go b/pkg/picod/server.go index d91fd91f6..b0d69fc02 100644 --- a/pkg/picod/server.go +++ b/pkg/picod/server.go @@ -79,9 +79,9 @@ func NewServer(config Config) *Server { engine := gin.New() // Global middleware - engine.Use(gin.Logger()) // Request logging - engine.Use(gin.Recovery()) // Crash recovery + engine.Use(gin.Logger()) // Request logging engine.Use(s.metrics.Middleware()) + engine.Use(gin.Recovery()) // Crash recovery engine.Use(maxBodySizeMiddleware()) engine.MaxMultipartMemory = MaxBodySize engine.Use(gzip.Gzip(gzip.BestSpeed, gzip.WithExcludedPaths([]string{"/health", "/metrics"}))) // Response compression From b8c4ed585ebac6413d98ff6f2479b45c84cf2961 Mon Sep 17 00:00:00 2001 From: Vanshika Date: Wed, 15 Jul 2026 09:56:20 +0530 Subject: [PATCH 4/4] fix(picod): normalize HTTP method labels and extend histogram buckets Prevent unbounded Prometheus cardinality from arbitrary HTTP method tokens. Add normalizeMethod() which maps the nine standard RFC methods through unchanged and collapses everything else to "unknown", then wire it into the metrics middleware. Extend picod_http_request_duration_seconds from prometheus.DefBuckets (max 10 s) to a 15-bucket set that covers the full 60-second default command timeout used by ExecuteHandler. Add TestNormalizeMethod to cover all standard and non-standard inputs. Add TestMetrics_UnknownMethod to verify that N distinct custom methods produce exactly one "unknown" series rather than N new series. Update verifyHTTPRequestDuration to assert the 60-second upper bucket is present. Signed-off-by: Vanshika --- pkg/picod/metrics.go | 31 ++++++++++++++++-- pkg/picod/metrics_test.go | 68 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/pkg/picod/metrics.go b/pkg/picod/metrics.go index 534303789..3ca0505c7 100644 --- a/pkg/picod/metrics.go +++ b/pkg/picod/metrics.go @@ -17,6 +17,7 @@ limitations under the License. package picod import ( + "net/http" "strconv" "time" @@ -25,6 +26,29 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" ) +const methodUnknown = "unknown" + +// requestDurationBuckets covers PicoD's full latency range: sub-millisecond +// for fast file and health-style paths through the 60-second default command +// timeout used by ExecuteHandler. +var requestDurationBuckets = []float64{ + 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, + 1, 2.5, 5, 10, 20, 30, 45, 60, +} + +// normalizeMethod maps non-standard HTTP methods to "unknown" to prevent +// unbounded label cardinality in Prometheus time series. +func normalizeMethod(method string) string { + switch method { + case http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete, + http.MethodPatch, http.MethodHead, http.MethodOptions, + http.MethodConnect, http.MethodTrace: + return method + default: + return methodUnknown + } +} + // Metrics holds the Prometheus collectors for PicoD. type Metrics struct { Registry *prometheus.Registry @@ -63,7 +87,7 @@ func NewMetrics() *Metrics { prometheus.HistogramOpts{ Name: "picod_http_request_duration_seconds", Help: "Latency of HTTP requests, partitioned by method and path.", - Buckets: prometheus.DefBuckets, + Buckets: requestDurationBuckets, }, []string{"method", "path"}, ) @@ -108,7 +132,8 @@ func (m *Metrics) Middleware() gin.HandlerFunc { duration := time.Since(start).Seconds() status := strconv.Itoa(c.Writer.Status()) - m.HTTPRequestsTotal.WithLabelValues(c.Request.Method, path, status).Inc() - m.HTTPRequestDuration.WithLabelValues(c.Request.Method, path).Observe(duration) + method := normalizeMethod(c.Request.Method) + m.HTTPRequestsTotal.WithLabelValues(method, path, status).Inc() + m.HTTPRequestDuration.WithLabelValues(method, path).Observe(duration) } } diff --git a/pkg/picod/metrics_test.go b/pkg/picod/metrics_test.go index 1ca2a9870..b24b9d16c 100644 --- a/pkg/picod/metrics_test.go +++ b/pkg/picod/metrics_test.go @@ -159,6 +159,12 @@ func verifyHTTPRequestDuration(t *testing.T, mf *dto.MetricFamily) { foundExecute = true assert.Greater(t, *m.Histogram.SampleCount, uint64(0)) assert.Greater(t, *m.Histogram.SampleSum, 0.0) + // Verify the extended bucket set was applied: all 15 explicit + // buckets must be present and the uppermost must cover the + // 60-second default command timeout. + require.Equal(t, len(requestDurationBuckets), len(m.Histogram.Bucket)) + lastBucket := m.Histogram.Bucket[len(m.Histogram.Bucket)-1] + assert.Equal(t, 60.0, *lastBucket.UpperBound) } } assert.True(t, foundExecute, "should record duration for POST /api/execute") @@ -240,3 +246,65 @@ func TestMetrics_UnmatchedRoute(t *testing.T) { } assert.True(t, foundUnmatched, "unmatched routes must use path label 'unmatched', not the raw URL path") } + +func TestNormalizeMethod(t *testing.T) { + standard := []string{ + http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete, + http.MethodPatch, http.MethodHead, http.MethodOptions, + http.MethodConnect, http.MethodTrace, + } + for _, m := range standard { + assert.Equal(t, m, normalizeMethod(m), "standard method must be preserved: %s", m) + } + + custom := []string{"FOO", "BAR", "RANDOM123", "PROPFIND", "MKCOL"} + for _, m := range custom { + assert.Equal(t, methodUnknown, normalizeMethod(m), "non-standard method must become %q: %s", methodUnknown, m) + } +} + +func TestMetrics_UnknownMethod(t *testing.T) { + _, routerPubStr := generateRSAKeys(t) + server, ts, tmpDir := setupTestServer(t, routerPubStr) + defer os.RemoveAll(tmpDir) + defer ts.Close() + defer os.Unsetenv(PublicKeyEnvVar) + + // Send three requests each with a distinct non-standard HTTP method. + customMethods := []string{"FOO", "BAR", "RANDOM123"} + for _, m := range customMethods { + w := httptest.NewRecorder() + r := httptest.NewRequest(m, executeAPIPath, nil) + server.engine.ServeHTTP(w, r) + } + + metricFamilies, err := server.metrics.Registry.Gather() + require.NoError(t, err) + + var unknownSeries int + var unknownTotal float64 + var customLabelFound bool + for _, mf := range metricFamilies { + if *mf.Name != metricHTTPRequestsTotal { + continue + } + for _, m := range mf.Metric { + for _, label := range m.Label { + if *label.Name != labelMethod { + continue + } + switch *label.Value { + case methodUnknown: + unknownSeries++ + unknownTotal += *m.Counter.Value + case "FOO", "BAR", "RANDOM123": + customLabelFound = true + } + } + } + } + + assert.Equal(t, 1, unknownSeries, "all non-standard methods must collapse into a single %q series", methodUnknown) + assert.Equal(t, float64(len(customMethods)), unknownTotal, "counter must reflect all requests in the single series") + assert.False(t, customLabelFound, "non-standard method names must not appear as metric label values") +}