From aafea55976b7537ededd4ebf88c97fe8b99fb3a5 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Dieguez Date: Tue, 14 Apr 2020 14:15:00 +0200 Subject: [PATCH] MM-23131 Include HTTP status code in the metrics (#14240) * ResponseWriter wrapper to get status code For our metrics, we need the status code returned by a request so this wrapper includes a new method StatusCode() that includes the desired code * Shadow the responsewriter variable in the handlers In order to avoid confusion to people deciding what variable to use. I've also changed the tests to reflect this change and added a new one that checks the Flush method works --- einterfaces/metrics.go | 2 +- einterfaces/mocks/MetricsInterface.go | 6 +- web/handlers.go | 5 +- web/response_writer_wrapper.go | 62 +++++++++++++++++++ web/response_writer_wrapper_test.go | 87 +++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 web/response_writer_wrapper.go create mode 100644 web/response_writer_wrapper_test.go diff --git a/einterfaces/metrics.go b/einterfaces/metrics.go index 5fa4816540..a68cec3dc9 100644 --- a/einterfaces/metrics.go +++ b/einterfaces/metrics.go @@ -48,7 +48,7 @@ type MetricsInterface interface { IncrementPostsSearchCounter() ObservePostsSearchDuration(elapsed float64) ObserveStoreMethodDuration(method, success string, elapsed float64) - ObserveApiEndpointDuration(endpoint, method string, elapsed float64) + ObserveApiEndpointDuration(endpoint, method, statusCode string, elapsed float64) IncrementPostIndexCounter() IncrementUserIndexCounter() IncrementChannelIndexCounter() diff --git a/einterfaces/mocks/MetricsInterface.go b/einterfaces/mocks/MetricsInterface.go index 13b69fdd7c..f6682bae73 100644 --- a/einterfaces/mocks/MetricsInterface.go +++ b/einterfaces/mocks/MetricsInterface.go @@ -171,9 +171,9 @@ func (_m *MetricsInterface) IncrementWebsocketEvent(eventType string) { _m.Called(eventType) } -// ObserveApiEndpointDuration provides a mock function with given fields: endpoint, method, elapsed -func (_m *MetricsInterface) ObserveApiEndpointDuration(endpoint string, method string, elapsed float64) { - _m.Called(endpoint, method, elapsed) +// ObserveApiEndpointDuration provides a mock function with given fields: endpoint, method, statusCode, elapsed +func (_m *MetricsInterface) ObserveApiEndpointDuration(endpoint string, method string, statusCode string, elapsed float64) { + _m.Called(endpoint, method, statusCode, elapsed) } // ObserveClusterRequestDuration provides a mock function with given fields: elapsed diff --git a/web/handlers.go b/web/handlers.go index 670ae7d1a4..e3cf169059 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -10,6 +10,7 @@ import ( "net/http" "reflect" "runtime" + "strconv" "strings" "time" @@ -79,6 +80,7 @@ type Handler struct { } func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + w = newWrappedWriter(w) now := time.Now() requestID := model.NewId() @@ -257,8 +259,9 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if r.URL.Path != model.API_URL_SUFFIX+"/websocket" { elapsed := float64(time.Since(now)) / float64(time.Second) + statusCode := strconv.Itoa(w.(*responseWriterWrapper).StatusCode()) c.App.Metrics().ObserveHttpRequestDuration(elapsed) - c.App.Metrics().ObserveApiEndpointDuration(h.HandlerName, r.Method, elapsed) + c.App.Metrics().ObserveApiEndpointDuration(h.HandlerName, r.Method, statusCode, elapsed) } } } diff --git a/web/response_writer_wrapper.go b/web/response_writer_wrapper.go new file mode 100644 index 0000000000..925a80257c --- /dev/null +++ b/web/response_writer_wrapper.go @@ -0,0 +1,62 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package web + +import ( + "bufio" + "errors" + "net" + "net/http" +) + +type responseWriterWrapper struct { + http.ResponseWriter + statusCode int + statusCodeWritten bool + hijacker http.Hijacker + flusher http.Flusher +} + +func newWrappedWriter(original http.ResponseWriter) *responseWriterWrapper { + hijacker, _ := original.(http.Hijacker) + flusher, _ := original.(http.Flusher) + return &responseWriterWrapper{ + ResponseWriter: original, + statusCodeWritten: false, + hijacker: hijacker, + flusher: flusher, + } +} + +func (rw *responseWriterWrapper) StatusCode() int { + return rw.statusCode +} + +func (rw *responseWriterWrapper) WriteHeader(statusCode int) { + rw.statusCode = statusCode + rw.statusCodeWritten = true + rw.ResponseWriter.WriteHeader(statusCode) +} + +func (rw *responseWriterWrapper) Write(data []byte) (int, error) { + if !rw.statusCodeWritten { + rw.statusCode = http.StatusOK + } + return rw.ResponseWriter.Write(data) +} + +// Using as embedded makes the ResponseWrite be stored as interface and that way +// it loses the access to the implementation for Hijack or Flush +func (rw *responseWriterWrapper) Hijack() (net.Conn, *bufio.ReadWriter, error) { + if rw.hijacker == nil { + return nil, nil, errors.New("Hijacker interface not supported by the wrapped ResponseWriter") + } + return rw.hijacker.Hijack() +} + +func (rw *responseWriterWrapper) Flush() { + if rw.flusher != nil { + rw.flusher.Flush() + } +} diff --git a/web/response_writer_wrapper_test.go b/web/response_writer_wrapper_test.go new file mode 100644 index 0000000000..0533a9105a --- /dev/null +++ b/web/response_writer_wrapper_test.go @@ -0,0 +1,87 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package web + +import ( + "bufio" + "net" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +type TestHandler struct { + TestFunc func(w http.ResponseWriter, r *http.Request) +} + +func (h *TestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + h.TestFunc(w, r) +} + +type responseRecorderHijack struct { + httptest.ResponseRecorder +} + +func (r *responseRecorderHijack) Hijack() (net.Conn, *bufio.ReadWriter, error) { + r.WriteHeader(http.StatusOK) + return nil, nil, nil +} + +func newResponseWithHijack(original *httptest.ResponseRecorder) *responseRecorderHijack { + return &responseRecorderHijack{*original} +} + +func TestStatusCodeIsAccessible(t *testing.T) { + resp := newWrappedWriter(httptest.NewRecorder()) + req := httptest.NewRequest("GET", "/api/v4/test", nil) + handler := TestHandler{func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + }} + handler.ServeHTTP(resp, req) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode()) +} + +func TestStatusCodeShouldBe200IfNotHeaderWritten(t *testing.T) { + resp := newWrappedWriter(httptest.NewRecorder()) + req := httptest.NewRequest("GET", "/api/v4/test", nil) + handler := TestHandler{func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte{}) + }} + handler.ServeHTTP(resp, req) + assert.Equal(t, http.StatusOK, resp.StatusCode()) +} + +func TestForUnsupportedHijack(t *testing.T) { + resp := newWrappedWriter(httptest.NewRecorder()) + req := httptest.NewRequest("GET", "/api/v4/test", nil) + handler := TestHandler{func(w http.ResponseWriter, r *http.Request) { + _, _, err := w.(*responseWriterWrapper).Hijack() + assert.NotNil(t, err) + assert.Equal(t, "Hijacker interface not supported by the wrapped ResponseWriter", err.Error()) + }} + handler.ServeHTTP(resp, req) +} + +func TestForSupportedHijack(t *testing.T) { + resp := newWrappedWriter(newResponseWithHijack(httptest.NewRecorder())) + req := httptest.NewRequest("GET", "/api/v4/test", nil) + handler := TestHandler{func(w http.ResponseWriter, r *http.Request) { + _, _, err := w.(*responseWriterWrapper).Hijack() + assert.Nil(t, err) + }} + handler.ServeHTTP(resp, req) +} + +func TestForSupportedFlush(t *testing.T) { + resp := newWrappedWriter(httptest.NewRecorder()) + req := httptest.NewRequest("GET", "/api/v4/test", nil) + handler := TestHandler{func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte{}) + w.(*responseWriterWrapper).Flush() + }} + handler.ServeHTTP(resp, req) + assert.Equal(t, http.StatusOK, resp.StatusCode()) +}