diff --git a/metrics/metrics.go b/metrics/metrics.go index a7ce1ba..a115826 100644 --- a/metrics/metrics.go +++ b/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, diff --git a/server/server.go b/server/server.go index 50886cd..862ac9e 100644 --- a/server/server.go +++ b/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)) diff --git a/server/server_test.go b/server/server_test.go index b8b872a..863375a 100644 --- a/server/server_test.go +++ b/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) + }) + } }