Fix metric middleware panic (#133)

* test: add server_test middleware test

* fix: revert to old put/get handler behavior, and fix tests

* fix: metric panic

* fix: lint

Author: samlaf

metrics/metrics.go

@@ -99,7 +99,7 @@ func NewMetrics(subsystem string) *Metrics {
 			Buckets: prometheus.ExponentialBucketsRange(0.05, 1200, 20),
 			Help:    "Histogram of HTTP server request durations",
 		}, []string{
-			"method", "commitment_mode", "DA_cert_version", // no status on histograms because those are very expensive
+			"method", // no status on histograms because those are very expensive
 		}),
 		registry: registry,
 		factory:  factory,

server/server.go

@@ -146,19 +146,23 @@ func (svr *Server) Health(w http.ResponseWriter, _ *http.Request) error {
 	return nil
 }
 
+// HandleGet handles the GET request for commitments.
+// Note: even when an error is returned, the commitment meta is still returned,
+// because it is needed for metrics (see the WithMetrics middleware).
+// TODO: we should change this behavior and instead use a custom error that contains the commitment meta.
 func (svr *Server) HandleGet(w http.ResponseWriter, r *http.Request) (commitments.CommitmentMeta, error) {
 	meta, err := ReadCommitmentMeta(r)
 	if err != nil {
 		err = fmt.Errorf("invalid commitment mode: %w", err)
 		svr.WriteBadRequest(w, err)
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 	key := path.Base(r.URL.Path)
 	comm, err := commitments.StringToDecodedCommitment(key, meta.Mode)
 	if err != nil {
 		err = fmt.Errorf("failed to decode commitment from key %v (commitment mode %v): %w", key, meta.Mode, err)
 		svr.WriteBadRequest(w, err)
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 
 	input, err := svr.router.Get(r.Context(), comm, meta.Mode)
@@ -169,26 +173,30 @@ func (svr *Server) HandleGet(w http.ResponseWriter, r *http.Request) (commitment
 		} else {
 			svr.WriteInternalError(w, err)
 		}
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 
 	svr.WriteResponse(w, input)
 	return meta, nil
 }
 
+// HandlePut handles the PUT request for commitments.
+// Note: even when an error is returned, the commitment meta is still returned,
+// because it is needed for metrics (see the WithMetrics middleware).
+// TODO: we should change this behavior and instead use a custom error that contains the commitment meta.
 func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitments.CommitmentMeta, error) {
 	meta, err := ReadCommitmentMeta(r)
 	if err != nil {
 		err = fmt.Errorf("invalid commitment mode: %w", err)
 		svr.WriteBadRequest(w, err)
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 
 	input, err := io.ReadAll(r.Body)
 	if err != nil {
 		err = fmt.Errorf("failed to read request body: %w", err)
 		svr.WriteBadRequest(w, err)
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 
 	key := path.Base(r.URL.Path)
@@ -199,7 +207,7 @@ func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitment
 		if err != nil {
 			err = fmt.Errorf("failed to decode commitment from key %v (commitment mode %v): %w", key, meta.Mode, err)
 			svr.WriteBadRequest(w, err)
-			return commitments.CommitmentMeta{}, err
+			return meta, err
 		}
 	}
 
@@ -214,7 +222,7 @@ func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitment
 	if err != nil {
 		err = fmt.Errorf("failed to encode commitment %v (commitment mode %v): %w", commitment, meta.Mode, err)
 		svr.WriteInternalError(w, err)
-		return commitments.CommitmentMeta{}, err
+		return meta, err
 	}
 
 	svr.log.Info(fmt.Sprintf("write commitment: %x\n", comm))

server/server_test.go

@@ -31,7 +31,8 @@ func TestGetHandler(t *testing.T) {
 
 	mockRouter := mocks.NewMockIRouter(ctrl)
 
-	server := NewServer("localhost", 8080, mockRouter, log.New(), metrics.NoopMetrics)
+	m := metrics.NewMetrics("default")
+	server := NewServer("localhost", 8080, mockRouter, log.New(), m)
 
 	tests := []struct {
 		name                   string
@@ -84,7 +85,7 @@ func TestGetHandler(t *testing.T) {
 			expectedCode:           http.StatusInternalServerError,
 			expectedBody:           "",
 			expectError:            true,
-			expectedCommitmentMeta: commitments.CommitmentMeta{},
+			expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismGeneric, CertVersion: 0},
 		},
 		{
 			name: "Success - OP Keccak256",
@@ -106,7 +107,7 @@ func TestGetHandler(t *testing.T) {
 			expectedCode:           http.StatusInternalServerError,
 			expectedBody:           "",
 			expectError:            true,
-			expectedCommitmentMeta: commitments.CommitmentMeta{},
+			expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismAltDA, CertVersion: 0},
 		},
 		{
 			name: "Success - OP Alt-DA",
@@ -138,6 +139,20 @@ func TestGetHandler(t *testing.T) {
 			require.Equal(t, tt.expectedCode, rec.Code)
 			require.Equal(t, tt.expectedCommitmentMeta, meta)
 			require.Equal(t, tt.expectedBody, rec.Body.String())
+
+		})
+	}
+	for _, tt := range tests {
+		t.Run(tt.name+"/CheckMiddlewaresNoPanic", func(_ *testing.T) {
+			tt.mockBehavior()
+
+			req := httptest.NewRequest(http.MethodGet, tt.url, nil)
+			rec := httptest.NewRecorder()
+			// we also run the request through the middlewares, to make sure no panic occurs
+			// could happen if there's a problem with the metrics. For eg, in the past we saw
+			// panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"}
+			handler := WithLogging(WithMetrics(server.HandleGet, server.m), server.log)
+			handler(rec, req)
 		})
 	}
 }
@@ -266,4 +281,18 @@ func TestPutHandler(t *testing.T) {
 			require.Equal(t, tt.expectedCommitmentMeta, meta)
 		})
 	}
+
+	for _, tt := range tests {
+		t.Run(tt.name+"/CheckMiddlewaresNoPanic", func(_ *testing.T) {
+			tt.mockBehavior()
+
+			req := httptest.NewRequest(http.MethodGet, tt.url, nil)
+			rec := httptest.NewRecorder()
+			// we also run the request through the middlewares, to make sure no panic occurs
+			// could happen if there's a problem with the metrics. For eg, in the past we saw
+			// panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"}
+			handler := WithLogging(WithMetrics(server.HandlePut, server.m), server.log)
+			handler(rec, req)
+		})
+	}
 }