From 958aa267cfa117cff7b2ad3bba89e75884fe2596 Mon Sep 17 00:00:00 2001 From: ionutboangiu Date: Wed, 17 Jul 2024 23:47:06 +0300 Subject: [PATCH] Refactor HTTP profiling and flag handling - hardcode the base path for profiling endpoints to '/debug/pprof/' - change profiling flag 'httprof_path' to boolean 'http_pprof' (because of the above) - remove redundant profiling endpoint registrations (handled by pprof.Index) - move profiling registration log after actual registration - make profiling registration log more descriptive - use utils.Logger instead of log.Print for the log mentioned above - refactor flags test into a table test (adding also verification for default flag values) - change 'scheduledShutdown' flag type from string to time.Duration (to avoid unnecessary time parsing) - revise flags usage descriptions - rename flag 'singlecpu' to 'singleCPU' - switch to 'ExitOnError' for flag parsing to simplify error handling and automatically handle 'ErrHelp' by exiting with status 0 when help is requested and status 2 for other parsing errors. Before the following error log would have been received: ' error received: , exiting!' - update cgr-engine documentation --- cmd/cgr-engine/cgr-engine.go | 51 +++--- cmd/cgr-engine/cgr-engine_flags_test.go | 217 ++++++++++++++---------- cores/server.go | 27 +-- cores/server_test.go | 2 +- docs/cgr-engine.rst | 39 ++--- utils/consts.go | 2 +- 6 files changed, 183 insertions(+), 155 deletions(-) diff --git a/cmd/cgr-engine/cgr-engine.go b/cmd/cgr-engine/cgr-engine.go index 414241284..457b44d0c 100644 --- a/cmd/cgr-engine/cgr-engine.go +++ b/cmd/cgr-engine/cgr-engine.go @@ -50,22 +50,22 @@ import ( ) var ( - cgrEngineFlags = flag.NewFlagSet(utils.CgrEngine, flag.ContinueOnError) - cfgPath = cgrEngineFlags.String(utils.CfgPathCgr, utils.ConfigPath, "Configuration directory path.") - version = cgrEngineFlags.Bool(utils.VersionCgr, false, "Prints the application version.") - printConfig = cgrEngineFlags.Bool(utils.PrintCfgCgr, false, "Print the configuration object in JSON format") - pidFile = cgrEngineFlags.String(utils.PidCgr, utils.EmptyString, "Write pid file") - httpPprofPath = cgrEngineFlags.String(utils.HttpPrfPthCgr, utils.EmptyString, "http address used for program profiling") - cpuProfDir = cgrEngineFlags.String(utils.CpuProfDirCgr, utils.EmptyString, "write cpu profile to files") - memProfDir = cgrEngineFlags.String(utils.MemProfDirCgr, utils.EmptyString, "write memory profile to file") - memProfInterval = cgrEngineFlags.Duration(utils.MemProfIntervalCgr, 5*time.Second, "Time between memory profile saves") - memProfNrFiles = cgrEngineFlags.Int(utils.MemProfNrFilesCgr, 1, "Number of memory profile to write") - scheduledShutdown = cgrEngineFlags.String(utils.ScheduledShutdownCgr, utils.EmptyString, "shutdown the engine after this duration") - singlecpu = cgrEngineFlags.Bool(utils.SingleCpuCgr, false, "Run on single CPU core") - syslogger = cgrEngineFlags.String(utils.LoggerCfg, utils.EmptyString, "logger <*syslog|*stdout>") - nodeID = cgrEngineFlags.String(utils.NodeIDCfg, utils.EmptyString, "The node ID of the engine") - logLevel = cgrEngineFlags.Int(utils.LogLevelCfg, -1, "Log level (0-emergency to 7-debug)") - preload = cgrEngineFlags.String(utils.PreloadCgr, utils.EmptyString, "LoaderIDs used to load the data before the engine starts") + cgrEngineFlags = flag.NewFlagSet(utils.CgrEngine, flag.ExitOnError) + cfgPath = cgrEngineFlags.String(utils.CfgPathCgr, utils.ConfigPath, "Configuration directory path") + version = cgrEngineFlags.Bool(utils.VersionCgr, false, "Print application version and exit") + printConfig = cgrEngineFlags.Bool(utils.PrintCfgCgr, false, "Print configuration object in JSON format") + pidFile = cgrEngineFlags.String(utils.PidCgr, utils.EmptyString, "Path to write the PID file") + httpPprof = cgrEngineFlags.Bool(utils.HttpPprofCgr, false, "Enable HTTP pprof profiling") + cpuProfDir = cgrEngineFlags.String(utils.CpuProfDirCgr, utils.EmptyString, "Directory for CPU profiles") + memProfDir = cgrEngineFlags.String(utils.MemProfDirCgr, utils.EmptyString, "Directory for memory profiles") + memProfInterval = cgrEngineFlags.Duration(utils.MemProfIntervalCgr, 5*time.Second, "Interval between memory profile saves") + memProfNrFiles = cgrEngineFlags.Int(utils.MemProfNrFilesCgr, 1, "Number of memory profiles to keep (most recent)") + scheduledShutdown = cgrEngineFlags.Duration(utils.ScheduledShutdownCgr, 0, "Shutdown the engine after the specified duration") + singleCPU = cgrEngineFlags.Bool(utils.SingleCpuCgr, false, "Run on a single CPU core") + syslogger = cgrEngineFlags.String(utils.LoggerCfg, utils.EmptyString, "Logger type <*syslog|*stdout>") + nodeID = cgrEngineFlags.String(utils.NodeIDCfg, utils.EmptyString, "Node ID of the engine") + logLevel = cgrEngineFlags.Int(utils.LogLevelCfg, -1, "Log level (0=emergency to 7=debug)") + preload = cgrEngineFlags.String(utils.PreloadCgr, utils.EmptyString, "Loader IDs used to load data before engine starts") setVersions = cgrEngineFlags.Bool("set_versions", false, "Overwrite database versions (equivalent to cgr-migrator -exec=*set_versions)") cfg *config.CGRConfig @@ -330,9 +330,7 @@ func runPreload(loader *services.LoaderService, internalLoaderSChan chan birpc.C } func main() { - if err := cgrEngineFlags.Parse(os.Args[1:]); err != nil { - log.Fatalf("<%s> error received: <%s>, exiting!", utils.InitS, err.Error()) - } + cgrEngineFlags.Parse(os.Args[1:]) vers, err := utils.GetCGRVersion() if err != nil { log.Fatalf("<%s> error received: <%s>, exiting!", utils.InitS, err.Error()) @@ -345,7 +343,7 @@ func main() { if *pidFile != utils.EmptyString { writePid() } - if *singlecpu { + if *singleCPU { runtime.GOMAXPROCS(1) // Having multiple cpus may slow down computing due to CPU management, to be reviewed in future Go releases } @@ -389,14 +387,10 @@ func main() { }() } - if *scheduledShutdown != utils.EmptyString { - shutdownDur, err := utils.ParseDurationWithNanosecs(*scheduledShutdown) - if err != nil { - log.Fatalf("<%s> error received: <%s>, exiting!", utils.InitS, err.Error()) - } + if *scheduledShutdown != 0 { shdWg.Add(1) go func() { // Schedule shutdown - tm := time.NewTimer(shutdownDur) + tm := time.NewTimer(*scheduledShutdown) select { case <-tm.C: shdChan.CloseOnce() @@ -563,8 +557,9 @@ func main() { if cfg.ConfigSCfg().Enabled { server.RegisterHttpFunc(cfg.ConfigSCfg().URL, config.HandlerConfigS) } - if *httpPprofPath != utils.EmptyString { - server.RegisterProfiler(*httpPprofPath) + if *httpPprof { + server.RegisterProfiler() + utils.Logger.Info(" registered profiling endpoints at '/debug/pprof/'") } // Define internal connections via channels diff --git a/cmd/cgr-engine/cgr-engine_flags_test.go b/cmd/cgr-engine/cgr-engine_flags_test.go index 56fae4800..4ca20a8ef 100644 --- a/cmd/cgr-engine/cgr-engine_flags_test.go +++ b/cmd/cgr-engine/cgr-engine_flags_test.go @@ -20,100 +20,141 @@ package main import ( "path" + "reflect" "testing" "time" ) -// if the flag change this should fail -// do not use constants in this test +// If any flag changes, this test should fail. +// Do not use constants in this test to ensure these changes are detected. func TestCgrEngineFlags(t *testing.T) { - if err := cgrEngineFlags.Parse([]string{"-config_path", path.Join("/conf", "samples", "tutorial")}); err != nil { - t.Fatal(err) - } else if *cfgPath != "/conf/samples/tutorial" { - t.Errorf("Expected /conf/samples/tutorial, received %+v", *cfgPath) + tests := []struct { + name string + flags []string + flagVar any + defaultVal any + want any + }{ + { + name: "cfgPath", + flags: []string{"-config_path", path.Join("/usr", "share", "cgrates", "conf", "samples", "tutorial")}, + flagVar: cfgPath, + defaultVal: "/etc/cgrates/", + want: "/usr/share/cgrates/conf/samples/tutorial", + }, + { + name: "version", + flags: []string{"-version"}, + flagVar: version, + defaultVal: false, + want: true, + }, + { + name: "printConfig", + flags: []string{"-print_config"}, + flagVar: printConfig, + defaultVal: false, + want: true, + }, + { + name: "pidFile", + flags: []string{"-pid", "/run/cgrates/cgrates.pid"}, + flagVar: pidFile, + defaultVal: "", + want: "/run/cgrates/cgrates.pid", + }, + { + name: "httpPprof", + flags: []string{"-http_pprof"}, + flagVar: httpPprof, + defaultVal: false, + want: true, + }, + { + name: "cpuProfDir", + flags: []string{"-cpuprof_dir", "/tmp/profiling"}, + flagVar: cpuProfDir, + defaultVal: "", + want: "/tmp/profiling", + }, + { + name: "memProfDir", + flags: []string{"-memprof_dir", "/tmp/profiling"}, + flagVar: memProfDir, + defaultVal: "", + want: "/tmp/profiling", + }, + { + name: "memProfInterval", + flags: []string{"-memprof_interval", "1s"}, + flagVar: memProfInterval, + defaultVal: 5 * time.Second, + want: time.Second, + }, + { + name: "memProfNrFiles", + flags: []string{"-memprof_nrfiles", "3"}, + flagVar: memProfNrFiles, + defaultVal: 1, + want: 3, + }, + { + name: "scheduledShutdown", + flags: []string{"-scheduled_shutdown", "1h"}, + flagVar: scheduledShutdown, + defaultVal: time.Duration(0), + want: time.Hour, + }, + { + name: "singleCPU", + flags: []string{"-singlecpu"}, + flagVar: singleCPU, + defaultVal: false, + want: true, + }, + { + name: "syslogger", + flags: []string{"-logger", "*stdout"}, + flagVar: syslogger, + defaultVal: "", + want: "*stdout", + }, + { + name: "nodeID", + flags: []string{"-node_id", "CGRateS.org"}, + flagVar: nodeID, + defaultVal: "", + want: "CGRateS.org", + }, + { + name: "logLevel", + flags: []string{"-log_level", "7"}, + flagVar: logLevel, + defaultVal: -1, + want: 7, + }, + { + name: "preload", + flags: []string{"-preload", "TestPreloadID"}, + flagVar: preload, + defaultVal: "", + want: "TestPreloadID", + }, } - if err := cgrEngineFlags.Parse([]string{"-version", "true"}); err != nil { - t.Fatal(err) - } else if *version != true { - t.Errorf("Expected true, received %+v", *version) - } - - if err := cgrEngineFlags.Parse([]string{"-print_config", "true"}); err != nil { - t.Fatal(err) - } else if *printConfig != true { - t.Errorf("Expected true, received %+v", *printConfig) - } - - if err := cgrEngineFlags.Parse([]string{"-pid", "usr/share/cgrates/cgrates.json"}); err != nil { - t.Fatal(err) - } else if *pidFile != "usr/share/cgrates/cgrates.json" { - t.Errorf("Expected usr/share/cgrates/cgrates.json, received %+v", *pidFile) - } - - if err := cgrEngineFlags.Parse([]string{"-httprof_path", "http://example.com/"}); err != nil { - t.Fatal(err) - } else if *httpPprofPath != "http://example.com/" { - t.Errorf("Expected http://example.com/, received %+v", *httpPprofPath) - } - - if err := cgrEngineFlags.Parse([]string{"-cpuprof_dir", "1"}); err != nil { - t.Fatal(err) - } else if *cpuProfDir != "1" { - t.Errorf("Expected 1, received %+v", *httpPprofPath) - } - - if err := cgrEngineFlags.Parse([]string{"-memprof_dir", "true"}); err != nil { - t.Fatal(err) - } else if *memProfDir != "true" { - t.Errorf("Expected true received %+v", *memProfDir) - } - - if err := cgrEngineFlags.Parse([]string{"-memprof_interval", "1s"}); err != nil { - t.Fatal(err) - } else if *memProfInterval != time.Second { - t.Errorf("Expected 1s, received %+v", *memProfInterval) - } - - if err := cgrEngineFlags.Parse([]string{"-memprof_nrfiles", "3"}); err != nil { - t.Fatal(err) - } else if *memProfNrFiles != 3 { - t.Errorf("Expected 3, received %+v", *memProfNrFiles) - } - - if err := cgrEngineFlags.Parse([]string{"-scheduled_shutdown", "1h"}); err != nil { - t.Fatal(err) - } else if *scheduledShutdown != "1h" { - t.Errorf("Expected 1h, received %+v", *scheduledShutdown) - } - - if err := cgrEngineFlags.Parse([]string{"-singlecpu"}); err != nil { - t.Fatal(err) - } else if *singlecpu != true { - t.Errorf("Expected true, received %+v", *singlecpu) - } - - if err := cgrEngineFlags.Parse([]string{"-logger", "*stdout"}); err != nil { - t.Fatal(err) - } else if *syslogger != "*stdout" { - t.Errorf("Expected *stdout, received %+v", *syslogger) - } - - if err := cgrEngineFlags.Parse([]string{"-node_id", "CGRates.org"}); err != nil { - t.Fatal(err) - } else if *nodeID != "CGRates.org" { - t.Errorf("Expected CGRates.org, received %+v", *nodeID) - } - - if err := cgrEngineFlags.Parse([]string{"-log_level", "7"}); err != nil { - t.Fatal(err) - } else if *logLevel != 7 { - t.Errorf("Expected 7, received %+v", *logLevel) - } - - if err := cgrEngineFlags.Parse([]string{"-preload", "TestPreloadID"}); err != nil { - t.Fatal(err) - } else if *preload != "TestPreloadID" { - t.Errorf("Expected 7, received %+v", *preload) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flagVal := reflect.ValueOf(tt.flagVar).Elem().Interface() + if flagVal != tt.defaultVal { + t.Errorf("%s=%v, want default value %v", tt.name, flagVal, tt.defaultVal) + } + if err := cgrEngineFlags.Parse(tt.flags); err != nil { + t.Errorf("cgrEngineFlags.Parse(%v) returned unexpected error: %v", tt.flags, err) + } + flagVal = reflect.ValueOf(tt.flagVar).Elem().Interface() + if flagVal != tt.want { + t.Errorf("%s=%v, want %v", tt.name, flagVal, tt.want) + } + }) } } diff --git a/cores/server.go b/cores/server.go index 79bec3b99..ed7ba146a 100644 --- a/cores/server.go +++ b/cores/server.go @@ -182,26 +182,17 @@ func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) { io.Copy(w, res) } -func registerProfiler(addr string, mux *http.ServeMux) { - mux.HandleFunc(addr, pprof.Index) - mux.HandleFunc(addr+"cmdline", pprof.Cmdline) - mux.HandleFunc(addr+"profile", pprof.Profile) - mux.HandleFunc(addr+"symbol", pprof.Symbol) - mux.HandleFunc(addr+"trace", pprof.Trace) - mux.Handle(addr+"goroutine", pprof.Handler("goroutine")) - mux.Handle(addr+"heap", pprof.Handler("heap")) - mux.Handle(addr+"threadcreate", pprof.Handler("threadcreate")) - mux.Handle(addr+"block", pprof.Handler("block")) - mux.Handle(addr+"allocs", pprof.Handler("allocs")) - mux.Handle(addr+"mutex", pprof.Handler("mutex")) +func registerProfiler(mux *http.ServeMux) { + mux.HandleFunc("/debug/pprof/", pprof.Index) + mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) + mux.HandleFunc("/debug/pprof/profile", pprof.Profile) + mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) + mux.HandleFunc("/debug/pprof/trace", pprof.Trace) } -func (s *Server) RegisterProfiler(addr string) { - if addr[len(addr)-1] != '/' { - addr = addr + "/" - } - registerProfiler(addr, s.httpMux) - registerProfiler(addr, s.httpsMux) +func (s *Server) RegisterProfiler() { + registerProfiler(s.httpMux) + registerProfiler(s.httpsMux) } func (s *Server) ServeHTTP(addr string, jsonRPCURL string, wsRPCURL string, diff --git a/cores/server_test.go b/cores/server_test.go index e4b624ee2..3493d1a76 100644 --- a/cores/server_test.go +++ b/cores/server_test.go @@ -99,7 +99,7 @@ func TestRegisterProfiler(t *testing.T) { caps := engine.NewCaps(0, utils.MetaBusy) rcv := NewServer(caps) - rcv.RegisterProfiler("/test_prefix") + rcv.RegisterProfiler() rcv.StopBiRPC() } diff --git a/docs/cgr-engine.rst b/docs/cgr-engine.rst index b536333ae..d4c0879c3 100644 --- a/docs/cgr-engine.rst +++ b/docs/cgr-engine.rst @@ -9,40 +9,41 @@ Customisable through the use of *json* :ref:`JSON configuration ` Able to read the configuration from either a local directory of *.json* files with an unlimited number of subfolders (ordered alphabetically) or a list of http paths (separated by ";"). + :: $ cgr-engine -help Usage of cgr-engine: - -check_config - Verify the config without starting the engine -config_path string - Configuration directory path. (default "/etc/cgrates/") + Configuration directory path (default "/etc/cgrates/") -cpuprof_dir string - write cpu profile to files - -httprof_path string - http address used for program profiling + Directory for CPU profiles + -http_pprof + Enable HTTP pprof profiling -log_level int - Log level (0-emergency to 7-debug) (default -1) + Log level (0=emergency to 7=debug) (default -1) -logger string - logger <*syslog|*stdout> + Logger type <*syslog|*stdout> -memprof_dir string - write memory profile to file + Directory for memory profiles -memprof_interval duration - Time between memory profile saves (default 5s) + Interval between memory profile saves (default 5s) -memprof_nrfiles int - Number of memory profile to write (default 1) + Number of memory profiles to keep (most recent) (default 1) -node_id string - The node ID of the engine + Node ID of the engine -pid string - Write pid file + Path to write the PID file -preload string - LoaderIDs used to load the data before the engine starts - -scheduled_shutdown string - shutdown the engine after this duration + Loader IDs used to load data before engine starts + -print_config + Print configuration object in JSON format + -scheduled_shutdown duration + Shutdown the engine after the specified duration -singlecpu - Run on single CPU core + Run on a single CPU core -version - Prints the application version. + Print application version and exit @@ -81,4 +82,4 @@ The components from the diagram can be found documented in the links bellow: caches datadb stordb - \ No newline at end of file + diff --git a/utils/consts.go b/utils/consts.go index 7483caa6b..5ae5df616 100644 --- a/utils/consts.go +++ b/utils/consts.go @@ -2881,7 +2881,7 @@ const ( CgrEngine = "cgr-engine" PrintCfgCgr = "print_config" PidCgr = "pid" - HttpPrfPthCgr = "httprof_path" + HttpPprofCgr = "http_pprof" CpuProfDirCgr = "cpuprof_dir" MemProfDirCgr = "memprof_dir" MemProfIntervalCgr = "memprof_interval"