[MM-48947] app/platform: do not restart metrics on every config save (#21831)

app/platform/config.go

@@ -75,12 +75,6 @@ func (ps *PlatformService) SaveConfig(newCfg *model.Config, sendConfigChangeClus
 		return nil, nil, model.NewAppError("saveConfig", "app.save_config.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
-	if ps.startMetrics && *ps.Config().MetricsSettings.Enable {
-		ps.RestartMetrics()
-	} else {
-		ps.ShutdownMetrics()
-	}
-
 	if ps.clusterIFace != nil {
 		err := ps.clusterIFace.ConfigChanged(ps.configStore.RemoveEnvironmentOverrides(oldCfg),
 			ps.configStore.RemoveEnvironmentOverrides(newCfg), sendConfigChangeClusterMessage)

app/platform/config_test.go

@@ -70,4 +70,31 @@ func TestConfigSave(t *testing.T) {
 		updatedCfg := th.Service.Config()
 		assert.Equal(t, "http://newhost.me", *updatedCfg.ServiceSettings.SiteURL)
 	})
+
+	t.Run("do not restart the metrics server on a different type of config change", func(t *testing.T) {
+		th := Setup(t, StartMetrics())
+		defer th.TearDown()
+
+		metricsMock := &mocks.MetricsInterface{}
+		metricsMock.On("IncrementWebsocketEvent", mock.AnythingOfType("string")).Return()
+		metricsMock.On("IncrementWebSocketBroadcastBufferSize", mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return()
+		metricsMock.On("DecrementWebSocketBroadcastBufferSize", mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return()
+		metricsMock.On("Register").Return()
+		th.Service.metricsIFace = metricsMock
+
+		// Change a random config setting
+		cfg := th.Service.Config().Clone()
+		cfg.ThemeSettings.EnableThemeSelection = model.NewBool(!*cfg.ThemeSettings.EnableThemeSelection)
+		th.Service.SaveConfig(cfg, false)
+		metricsMock.AssertNumberOfCalls(t, "Register", 0)
+
+		// Disable metrics
+		cfg.MetricsSettings.Enable = model.NewBool(false)
+		th.Service.SaveConfig(cfg, false)
+
+		// Change the metrics setting
+		cfg.MetricsSettings.Enable = model.NewBool(true)
+		th.Service.SaveConfig(cfg, false)
+		metricsMock.AssertNumberOfCalls(t, "Register", 1)
+	})
 }

app/platform/service.go

@@ -268,6 +268,14 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) {
 		if mErr := ps.resetMetrics(); mErr != nil {
 			return nil, mErr
 		}
+
+		ps.configStore.AddListener(func(oldCfg, newCfg *model.Config) {
+			if *oldCfg.MetricsSettings.Enable != *newCfg.MetricsSettings.Enable || *oldCfg.MetricsSettings.ListenAddress != *newCfg.MetricsSettings.ListenAddress {
+				if mErr := ps.resetMetrics(); mErr != nil {
+					mlog.Warn("Failed to reset metrics", mlog.Err(mErr))
+				}
+			}
+		})
 	}
 
 	// Step 9: Init AsymmetricSigningKey depends on step 6 (store)

app/platform/service_test.go

@@ -105,10 +105,9 @@ func TestMetrics(t *testing.T) {
 
 		// there is no config listener for the metrics
 		// we handle it on config save step
-		th.Service.UpdateConfig(func(c *model.Config) {
-			c.MetricsSettings.Enable = model.NewBool(true)
-		})
-		th.Service.SaveConfig(th.Service.Config(), false)
+		cfg := th.Service.Config().Clone()
+		cfg.MetricsSettings.Enable = model.NewBool(true)
+		th.Service.SaveConfig(cfg, false)
 
 		require.NotNil(t, th.Service.metrics)
 		metricsAddr := strings.Replace(th.Service.metrics.listenAddr, "[::]", "http://localhost", 1)
@@ -117,17 +116,14 @@ func TestMetrics(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 
-		th.Service.UpdateConfig(func(c *model.Config) {
-			c.MetricsSettings.Enable = model.NewBool(false)
-		})
-		th.Service.SaveConfig(th.Service.Config(), false)
+		cfg.MetricsSettings.Enable = model.NewBool(false)
+		th.Service.SaveConfig(cfg, false)
 
 		_, err = http.Get(metricsAddr)
 		require.Error(t, err)
 	})
 
 	t.Run("ensure the metrics server is started with advanced metrics", func(t *testing.T) {
-		t.Skip("MM-47635")
 		th := Setup(t, StartMetrics())
 		defer th.TearDown()