MM-60269: Removed an incorrect return that caused missing API timing metrics for 4xx and 5xx errors (#28049)

* removed an incorretc return that caused missing API timing metrics for 4xx and 5xx errors

* Added tests and recorded metric for URL length limit as well

* Status code fix

server/channels/api4/metrics_test.go

@@ -155,4 +155,67 @@ func TestSubmitMetrics(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 	})
+
+	t.Run("metrics recorded for API errors", func(t *testing.T) {
+		metricsMock := setupMetricsMock()
+		metricsMock.On("IncrementClientLongTasks", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return()
+
+		platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface {
+			return metricsMock
+		})
+		t.Cleanup(func() {
+			platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface {
+				return nil
+			})
+		})
+
+		th := SetupEnterpriseWithServerOptions(t, []app.Option{app.StartMetrics})
+		defer th.TearDown()
+
+		// enable metrics and add the license
+		th.App.Srv().SetLicense(model.NewTestLicense())
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.MetricsSettings.Enable = true })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.MetricsSettings.ListenAddress = ":0" })
+
+		_, resp, err := th.Client.CreatePost(th.Context.Context(), &model.Post{
+			ChannelId: model.NewId(),
+		})
+
+		require.Error(t, err)
+		require.Equal(t, http.StatusForbidden, resp.StatusCode)
+		metricsMock.AssertCalled(t, "IncrementHTTPRequest")
+		metricsMock.AssertCalled(t, "IncrementHTTPError")
+	})
+
+	t.Run("metrics recorded for URL length limit errors", func(t *testing.T) {
+		metricsMock := setupMetricsMock()
+		metricsMock.On("IncrementClientLongTasks", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return()
+
+		platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface {
+			return metricsMock
+		})
+		t.Cleanup(func() {
+			platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface {
+				return nil
+			})
+		})
+
+		th := SetupEnterpriseWithServerOptions(t, []app.Option{app.StartMetrics})
+		defer th.TearDown()
+
+		// enable metrics and add the license
+		th.App.Srv().SetLicense(model.NewTestLicense())
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.MetricsSettings.Enable = true })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.MetricsSettings.ListenAddress = ":0" })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.MaximumURLLength = 1 })
+
+		_, resp, err := th.Client.CreatePost(th.Context.Context(), &model.Post{
+			ChannelId: model.NewId(),
+		})
+
+		require.Error(t, err)
+		require.Equal(t, http.StatusRequestURITooLong, resp.StatusCode)
+		metricsMock.AssertCalled(t, "IncrementHTTPRequest")
+		metricsMock.AssertCalled(t, "IncrementHTTPError")
+	})
 }

server/channels/web/handlers.go

@@ -164,7 +164,7 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	requestID := model.NewId()
-	var statusCode string
+	var rateLimitExceeded bool
 	defer func() {
 		responseLogFields := []mlog.Field{
 			mlog.String("method", r.Method),
@@ -175,11 +175,18 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		if c.AppContext.Session() != nil {
 			responseLogFields = append(responseLogFields, mlog.String("user_id", c.AppContext.Session().UserId))
 		}
+
+		statusCode := strconv.Itoa(w.(*responseWriterWrapper).StatusCode())
+
 		// Websockets are returning status code 0 to requests after closing the socket
 		if statusCode != "0" {
 			responseLogFields = append(responseLogFields, mlog.String("status_code", statusCode))
 		}
 		mlog.Debug("Received HTTP request", responseLogFields...)
+
+		if !rateLimitExceeded {
+			h.recordMetrics(c, r, now, statusCode)
+		}
 	}()
 
 	t, _ := i18n.GetTranslationsAndLocaleFromRequest(r)
@@ -302,8 +309,11 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 
 		// Rate limit by UserID
-		if c.App.Srv().RateLimiter != nil && c.App.Srv().RateLimiter.UserIdRateLimit(c.AppContext.Session().UserId, w) {
+		if c.App.Srv().RateLimiter != nil {
-			return
+			rateLimitExceeded = c.App.Srv().RateLimiter.UserIdRateLimit(c.AppContext.Session().UserId, w)
+			if rateLimitExceeded {
+				return
+			}
 		}
 
 		h.checkCSRFToken(c, r, token, tokenLocation, session)
@@ -382,8 +392,9 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		h.handleContextError(c, w, r)
 		return
 	}
+}
 
-	statusCode = strconv.Itoa(w.(*responseWriterWrapper).StatusCode())
+func (h Handler) recordMetrics(c *Context, r *http.Request, now time.Time, statusCode string) {
 	if c.App.Metrics() != nil {
 		c.App.Metrics().IncrementHTTPRequest()