From 90c635041053fc53905be5735b0399bfe135080e Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 28 Jul 2022 01:00:25 +0530 Subject: [PATCH] Revert "move metrics server into platform service (#20683)" (#20726) This reverts commit b18a42313b55dd74cf50cfd5f5ff19773f9395ea. Co-authored-by: Mattermod --- api4/apitestlib.go | 4 +- app/config.go | 4 +- app/platform/config.go | 14 ---- app/platform/metrics.go | 162 ---------------------------------------- app/platform/service.go | 31 +------- app/server.go | 145 ++++++++++++++++++++++++++++++----- 6 files changed, 135 insertions(+), 225 deletions(-) delete mode 100644 app/platform/metrics.go diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 9f2a91037d..e40bd255fd 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -322,8 +322,8 @@ func SetupWithStoreMock(tb testing.TB) *TestHelper { return th } -func SetupEnterpriseWithStoreMock(tb testing.TB, options ...app.Option) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil, options) +func SetupEnterpriseWithStoreMock(tb testing.TB) *TestHelper { + th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.StatusOnline}, nil) diff --git a/app/config.go b/app/config.go index 9fb2d92af2..bbbc92ff44 100644 --- a/app/config.go +++ b/app/config.go @@ -82,9 +82,9 @@ func (w *configWrapper) SaveConfig(newCfg *model.Config, sendConfigChangeCluster if w.srv.Metrics != nil { w.srv.Metrics.Register() } - w.srv.platformService.RestartMetrics() // TODO: remove when this moved to the platform service + w.srv.SetupMetricsServer() } else { - w.srv.platformService.ShutdownMetrics() // TODO: remove when this moved to the platform service + w.srv.StopMetricsServer() } if w.srv.Cluster != nil { diff --git a/app/platform/config.go b/app/platform/config.go index 37e7286283..2713166849 100644 --- a/app/platform/config.go +++ b/app/platform/config.go @@ -3,28 +3,14 @@ package platform -import ( - "errors" - - "github.com/mattermost/mattermost-server/v6/config" - "github.com/mattermost/mattermost-server/v6/einterfaces" -) - // ServiceConfig is used to initialize the PlatformService. // The mandatory fields will be checked during the initialization of the service. type ServiceConfig struct { // Mandatory fields - ConfigStore *config.Store - StartMetrics bool // TODO: find an elegant way to start/stop metrics server by default // Optional fields - Metrics einterfaces.MetricsInterface - Cluster einterfaces.ClusterInterface } func (c *ServiceConfig) validate() error { // Mandatory fields need to be checked here - if c.ConfigStore == nil { - return errors.New("ConfigStore is required") - } return nil } diff --git a/app/platform/metrics.go b/app/platform/metrics.go deleted file mode 100644 index e5daef5457..0000000000 --- a/app/platform/metrics.go +++ /dev/null @@ -1,162 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package platform - -import ( - "context" - "net" - "net/http" - "net/http/pprof" - "runtime" - "sync" - "text/template" - "time" - - "github.com/gorilla/handlers" - "github.com/gorilla/mux" - "github.com/mattermost/mattermost-server/v6/einterfaces" - "github.com/mattermost/mattermost-server/v6/model" - "github.com/mattermost/mattermost-server/v6/shared/mlog" - "github.com/pkg/errors" -) - -const TimeToWaitForConnectionsToCloseOnServerShutdown = time.Second - -type platformMetrics struct { - server *http.Server - router *mux.Router - lock sync.Mutex - - metricsImpl einterfaces.MetricsInterface - - cfgFn func() *model.Config -} - -func newPlatformMetrics(metricsImpl einterfaces.MetricsInterface, cfgFn func() *model.Config) *platformMetrics { - if !*cfgFn().MetricsSettings.Enable { - return nil - } - - pm := &platformMetrics{ - cfgFn: cfgFn, - } - - pm.stopMetricsServer() - - if err := pm.initMetricsRouter(); err != nil { - mlog.Error("Error initiating metrics router.", mlog.Err(err)) - } - - if metricsImpl != nil { - metricsImpl.Register() - } - - pm.startMetricsServer() - - return pm -} - -func (pm *platformMetrics) stopMetricsServer() { - pm.lock.Lock() - defer pm.lock.Unlock() - - if pm.server != nil { - ctx, cancel := context.WithTimeout(context.Background(), TimeToWaitForConnectionsToCloseOnServerShutdown) - defer cancel() - - pm.server.Shutdown(ctx) - mlog.Info("Metrics and profiling server is stopping") - } -} - -func (pm *platformMetrics) startMetricsServer() { - var notify chan struct{} - pm.lock.Lock() - defer func() { - if notify != nil { - <-notify - } - pm.lock.Unlock() - }() - - l, err := net.Listen("tcp", *pm.cfgFn().MetricsSettings.ListenAddress) - if err != nil { - mlog.Error(err.Error()) - return - } - - notify = make(chan struct{}) - pm.server = &http.Server{ - Handler: handlers.RecoveryHandler(handlers.PrintRecoveryStack(true))(pm.router), - ReadTimeout: time.Duration(*pm.cfgFn().ServiceSettings.ReadTimeout) * time.Second, - WriteTimeout: time.Duration(*pm.cfgFn().ServiceSettings.WriteTimeout) * time.Second, - } - - go func() { - close(notify) - if err := pm.server.Serve(l); err != nil && err != http.ErrServerClosed { - mlog.Critical(err.Error()) - } - }() - - mlog.Info("Metrics and profiling server is started", mlog.String("address", l.Addr().String())) -} - -func (pm *platformMetrics) initMetricsRouter() error { - pm.router = mux.NewRouter() - runtime.SetBlockProfileRate(*pm.cfgFn().MetricsSettings.BlockProfileRate) - - metricsPage := ` - - {{if .}} -
Metrics
{{end}} -
Profiling Root
-
Profiling Command Line
-
Profiling Symbols
-
Profiling Goroutines
-
Profiling Heap
-
Profiling Threads
-
Profiling Blocking
-
Profiling Execution Trace
-
Profiling CPU
- - - ` - metricsPageTmpl, err := template.New("page").Parse(metricsPage) - if err != nil { - return errors.Wrap(err, "failed to create template") - } - - rootHandler := func(w http.ResponseWriter, r *http.Request) { - metricsPageTmpl.Execute(w, pm.metricsImpl != nil) - } - - pm.router.HandleFunc("/", rootHandler) - pm.router.StrictSlash(true) - - pm.router.Handle("/debug", http.RedirectHandler("/", http.StatusMovedPermanently)) - pm.router.HandleFunc("/debug/pprof/", pprof.Index) - pm.router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) - pm.router.HandleFunc("/debug/pprof/profile", pprof.Profile) - pm.router.HandleFunc("/debug/pprof/symbol", pprof.Symbol) - pm.router.HandleFunc("/debug/pprof/trace", pprof.Trace) - - // Manually add support for paths linked to by index page at /debug/pprof/ - pm.router.Handle("/debug/pprof/goroutine", pprof.Handler("goroutine")) - pm.router.Handle("/debug/pprof/heap", pprof.Handler("heap")) - pm.router.Handle("/debug/pprof/threadcreate", pprof.Handler("threadcreate")) - pm.router.Handle("/debug/pprof/block", pprof.Handler("block")) - - return nil -} - -func (ps *PlatformService) HandleMetrics(route string, h http.Handler) { - if ps.metrics.router != nil { - ps.metrics.router.Handle(route, h) - } -} - -func (ps *PlatformService) RestartMetrics() { - ps.metrics = newPlatformMetrics(ps.serviceConfig.Metrics, ps.serviceConfig.ConfigStore.Get) -} diff --git a/app/platform/service.go b/app/platform/service.go index 7524a3a491..a9eda866f7 100644 --- a/app/platform/service.go +++ b/app/platform/service.go @@ -3,42 +3,17 @@ package platform -import ( - "github.com/mattermost/mattermost-server/v6/config" - "github.com/mattermost/mattermost-server/v6/einterfaces" -) - // PlatformService is the service for the platform related tasks. It is // responsible for non-entity related functionalities that are required // by a product such as database access, configuration access, licensing etc. type PlatformService struct { - serviceConfig ServiceConfig - configStore *config.Store - - metrics *platformMetrics - - cluster einterfaces.ClusterInterface } // New creates a new PlatformService. -func New(sc ServiceConfig) (*PlatformService, error) { - if err := sc.validate(); err != nil { +func New(c ServiceConfig) (*PlatformService, error) { + if err := c.validate(); err != nil { return nil, err } - ps := &PlatformService{ - serviceConfig: sc, - configStore: sc.ConfigStore, - cluster: sc.Cluster, - } - - ps.metrics = newPlatformMetrics(sc.Metrics, ps.configStore.Get) - - return ps, nil -} - -func (ps *PlatformService) ShutdownMetrics() { - if ps.metrics != nil { - ps.metrics.stopMetricsServer() - } + return &PlatformService{}, nil } diff --git a/app/server.go b/app/server.go index fddb9ef1c5..e363e76ca6 100644 --- a/app/server.go +++ b/app/server.go @@ -9,8 +9,10 @@ import ( "crypto/tls" "fmt" "hash/maphash" + "html/template" "net" "net/http" + "net/http/pprof" "net/url" "os" "os/exec" @@ -25,6 +27,7 @@ import ( "github.com/getsentry/sentry-go" sentryhttp "github.com/getsentry/sentry-go/http" + "github.com/gorilla/handlers" "github.com/gorilla/mux" "github.com/pkg/errors" "github.com/rs/cors" @@ -32,7 +35,6 @@ import ( "github.com/mattermost/mattermost-server/v6/app/email" "github.com/mattermost/mattermost-server/v6/app/featureflag" - "github.com/mattermost/mattermost-server/v6/app/platform" "github.com/mattermost/mattermost-server/v6/app/request" "github.com/mattermost/mattermost-server/v6/app/teams" "github.com/mattermost/mattermost-server/v6/app/users" @@ -129,6 +131,10 @@ type Server struct { localModeServer *http.Server + metricsServer *http.Server + metricsRouter *mux.Router + metricsLock sync.Mutex + didFinishListen chan struct{} goroutineCount int32 @@ -171,7 +177,6 @@ type Server struct { configStore *configWrapper filestore filestore.FileBackend - platformService *platform.PlatformService telemetryService *telemetry.TelemetryService userService *users.UserService teamService *teams.TeamService @@ -251,17 +256,6 @@ func NewServer(options ...Option) (*Server, error) { s.configStore = &configWrapper{srv: s, Store: configStore} } - ps, sErr := platform.New(platform.ServiceConfig{ - ConfigStore: s.configStore.Store, - StartMetrics: s.startMetrics, - Metrics: s.Metrics, - Cluster: s.Cluster, - }) - if sErr != nil { - return nil, errors.Wrap(sErr, "failed to initialize platform") - } - s.platformService = ps - // Step 2: Logging if err := s.initLogging(); err != nil { mlog.Error("Could not initiate logging", mlog.Err(err)) @@ -625,6 +619,10 @@ func NewServer(options ...Option) (*Server, error) { s.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableDeveloper = true }) } + if s.startMetrics { + s.SetupMetricsServer() + } + s.AddLicenseListener(func(oldLicense, newLicense *model.License) { if (oldLicense == nil && newLicense == nil) || !s.startMetrics { return @@ -634,7 +632,7 @@ func NewServer(options ...Option) (*Server, error) { return } - s.platformService.RestartMetrics() // TODO: remove when this moved to the platform service + s.SetupMetricsServer() }) s.SearchEngine.UpdateConfig(s.Config()) @@ -703,6 +701,24 @@ func NewServer(options ...Option) (*Server, error) { return s, nil } +func (s *Server) SetupMetricsServer() { + if !*s.Config().MetricsSettings.Enable { + return + } + + s.StopMetricsServer() + + if err := s.InitMetricsRouter(); err != nil { + mlog.Error("Error initiating metrics router.", mlog.Err(err)) + } + + if s.Metrics != nil { + s.Metrics.Register() + } + + s.startMetricsServer() +} + func maxInt(a, b int) int { if a > b { return a @@ -1030,7 +1046,7 @@ func (s *Server) Shutdown() { s.Cluster.StopInterNodeCommunication() } - s.platformService.ShutdownMetrics() + s.StopMetricsServer() // This must be done after the cluster is stopped. if s.Jobs != nil { @@ -1614,9 +1630,104 @@ func doConfigCleanup(s *Server) { } } -// TODO: remove this method when we switch to using platform service. +func (s *Server) StopMetricsServer() { + s.metricsLock.Lock() + defer s.metricsLock.Unlock() + + if s.metricsServer != nil { + ctx, cancel := context.WithTimeout(context.Background(), TimeToWaitForConnectionsToCloseOnServerShutdown) + defer cancel() + + s.metricsServer.Shutdown(ctx) + s.Log.Info("Metrics and profiling server is stopping") + } +} + func (s *Server) HandleMetrics(route string, h http.Handler) { - s.platformService.HandleMetrics(route, h) + if s.metricsRouter != nil { + s.metricsRouter.Handle(route, h) + } +} + +func (s *Server) InitMetricsRouter() error { + s.metricsRouter = mux.NewRouter() + runtime.SetBlockProfileRate(*s.Config().MetricsSettings.BlockProfileRate) + + metricsPage := ` + + {{if .}} +
Metrics
{{end}} +
Profiling Root
+
Profiling Command Line
+
Profiling Symbols
+
Profiling Goroutines
+
Profiling Heap
+
Profiling Threads
+
Profiling Blocking
+
Profiling Execution Trace
+
Profiling CPU
+ + + ` + metricsPageTmpl, err := template.New("page").Parse(metricsPage) + if err != nil { + return errors.Wrap(err, "failed to create template") + } + + rootHandler := func(w http.ResponseWriter, r *http.Request) { + metricsPageTmpl.Execute(w, s.Metrics != nil) + } + + s.metricsRouter.HandleFunc("/", rootHandler) + s.metricsRouter.StrictSlash(true) + + s.metricsRouter.Handle("/debug", http.RedirectHandler("/", http.StatusMovedPermanently)) + s.metricsRouter.HandleFunc("/debug/pprof/", pprof.Index) + s.metricsRouter.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) + s.metricsRouter.HandleFunc("/debug/pprof/profile", pprof.Profile) + s.metricsRouter.HandleFunc("/debug/pprof/symbol", pprof.Symbol) + s.metricsRouter.HandleFunc("/debug/pprof/trace", pprof.Trace) + + // Manually add support for paths linked to by index page at /debug/pprof/ + s.metricsRouter.Handle("/debug/pprof/goroutine", pprof.Handler("goroutine")) + s.metricsRouter.Handle("/debug/pprof/heap", pprof.Handler("heap")) + s.metricsRouter.Handle("/debug/pprof/threadcreate", pprof.Handler("threadcreate")) + s.metricsRouter.Handle("/debug/pprof/block", pprof.Handler("block")) + + return nil +} + +func (s *Server) startMetricsServer() { + var notify chan struct{} + s.metricsLock.Lock() + defer func() { + if notify != nil { + <-notify + } + s.metricsLock.Unlock() + }() + + l, err := net.Listen("tcp", *s.Config().MetricsSettings.ListenAddress) + if err != nil { + mlog.Error(err.Error()) + return + } + + notify = make(chan struct{}) + s.metricsServer = &http.Server{ + Handler: handlers.RecoveryHandler(handlers.PrintRecoveryStack(true))(s.metricsRouter), + ReadTimeout: time.Duration(*s.Config().ServiceSettings.ReadTimeout) * time.Second, + WriteTimeout: time.Duration(*s.Config().ServiceSettings.WriteTimeout) * time.Second, + } + + go func() { + close(notify) + if err := s.metricsServer.Serve(l); err != nil && err != http.ErrServerClosed { + mlog.Critical(err.Error()) + } + }() + + s.Log.Info("Metrics and profiling server is started", mlog.String("address", l.Addr().String())) } func (s *Server) sendLicenseUpForRenewalEmail(users map[string]*model.User, license *model.License) *model.AppError {