From 83b532c4dacbf1cf6342e8e696e50bc57e94effc Mon Sep 17 00:00:00 2001 From: lightclient Date: Thu, 21 Nov 2024 20:57:29 +0800 Subject: [PATCH 1/4] cmd/geth: actually enable metrics when passed via toml --- cmd/geth/chaincmd.go | 9 ++++---- cmd/geth/config.go | 5 +++++ cmd/geth/main.go | 7 ------- cmd/utils/flags.go | 49 ++++++++++++++++++++------------------------ 4 files changed, 31 insertions(+), 39 deletions(-) diff --git a/cmd/geth/chaincmd.go b/cmd/geth/chaincmd.go index f6dc1cf4bf0a..75bff7789e1c 100644 --- a/cmd/geth/chaincmd.go +++ b/cmd/geth/chaincmd.go @@ -282,14 +282,13 @@ func importChain(ctx *cli.Context) error { if ctx.Args().Len() < 1 { utils.Fatalf("This command requires an argument.") } + stack, cfg := makeConfigNode(ctx) + defer stack.Close() + // Start metrics export if enabled - utils.SetupMetrics(ctx) - // Start system runtime metrics collection + utils.SetupMetrics(&cfg.Metrics) go metrics.CollectProcessMetrics(3 * time.Second) - stack, _ := makeConfigNode(ctx) - defer stack.Close() - chain, db := utils.MakeChain(ctx, stack, false) defer db.Close() diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 17ed9fb606db..e1dc0fda311e 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -25,6 +25,7 @@ import ( "runtime" "slices" "strings" + "time" "unicode" "github.com/ethereum/go-ethereum/accounts" @@ -192,6 +193,10 @@ func makeFullNode(ctx *cli.Context) *node.Node { cfg.Eth.OverrideVerkle = &v } + // Start metrics export if enabled + utils.SetupMetrics(&cfg.Metrics) + go metrics.CollectProcessMetrics(3 * time.Second) + backend, eth := utils.RegisterEthService(stack, &cfg.Eth) // Create gauge with geth system and build information diff --git a/cmd/geth/main.go b/cmd/geth/main.go index 1527e180fb9f..9d9256862b63 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -34,7 +34,6 @@ import ( "github.com/ethereum/go-ethereum/internal/debug" "github.com/ethereum/go-ethereum/internal/flags" "github.com/ethereum/go-ethereum/log" - "github.com/ethereum/go-ethereum/metrics" "github.com/ethereum/go-ethereum/node" "go.uber.org/automaxprocs/maxprocs" @@ -325,12 +324,6 @@ func prepare(ctx *cli.Context) { ctx.Set(utils.CacheFlag.Name, strconv.Itoa(4096)) } } - - // Start metrics export if enabled - utils.SetupMetrics(ctx) - - // Start system runtime metrics collection - go metrics.CollectProcessMetrics(3 * time.Second) } // geth is the main entry point into the system if no special subcommand is run. diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index e635dd89c39b..002ab647f1ed 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -1969,24 +1969,19 @@ func RegisterFullSyncTester(stack *node.Node, eth *eth.Ethereum, target common.H log.Info("Registered full-sync tester", "hash", target) } -func SetupMetrics(ctx *cli.Context) { - if metrics.Enabled { +func SetupMetrics(cfg *metrics.Config) { + if cfg.Enabled { log.Info("Enabling metrics collection") - var ( - enableExport = ctx.Bool(MetricsEnableInfluxDBFlag.Name) - enableExportV2 = ctx.Bool(MetricsEnableInfluxDBV2Flag.Name) + enableExport = cfg.EnableInfluxDB + enableExportV2 = cfg.EnableInfluxDBV2 ) - + if enableExport && enableExportV2 { + Fatalf("Flags %v can't be used at the same time", strings.Join([]string{MetricsEnableInfluxDBFlag.Name, MetricsEnableInfluxDBV2Flag.Name}, ", ")) + } if enableExport || enableExportV2 { - CheckExclusive(ctx, MetricsEnableInfluxDBFlag, MetricsEnableInfluxDBV2Flag) - - v1FlagIsSet := ctx.IsSet(MetricsInfluxDBUsernameFlag.Name) || - ctx.IsSet(MetricsInfluxDBPasswordFlag.Name) - - v2FlagIsSet := ctx.IsSet(MetricsInfluxDBTokenFlag.Name) || - ctx.IsSet(MetricsInfluxDBOrganizationFlag.Name) || - ctx.IsSet(MetricsInfluxDBBucketFlag.Name) + v1FlagIsSet := cfg.InfluxDBUsername != "" || cfg.InfluxDBPassword != "" + v2FlagIsSet := cfg.InfluxDBToken != "" || cfg.InfluxDBOrganization != "" || cfg.InfluxDBBucket != "" if enableExport && v2FlagIsSet { Fatalf("Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2") @@ -1996,35 +1991,35 @@ func SetupMetrics(ctx *cli.Context) { } var ( - endpoint = ctx.String(MetricsInfluxDBEndpointFlag.Name) - database = ctx.String(MetricsInfluxDBDatabaseFlag.Name) - username = ctx.String(MetricsInfluxDBUsernameFlag.Name) - password = ctx.String(MetricsInfluxDBPasswordFlag.Name) - - token = ctx.String(MetricsInfluxDBTokenFlag.Name) - bucket = ctx.String(MetricsInfluxDBBucketFlag.Name) - organization = ctx.String(MetricsInfluxDBOrganizationFlag.Name) + endpoint = cfg.InfluxDBEndpoint + database = cfg.InfluxDBDatabase + username = cfg.InfluxDBUsername + password = cfg.InfluxDBPassword + + token = cfg.InfluxDBToken + bucket = cfg.InfluxDBBucket + organization = cfg.InfluxDBOrganization ) if enableExport { - tagsMap := SplitTagsFlag(ctx.String(MetricsInfluxDBTagsFlag.Name)) + tagsMap := SplitTagsFlag(cfg.InfluxDBTags) log.Info("Enabling metrics export to InfluxDB") go influxdb.InfluxDBWithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, database, username, password, "geth.", tagsMap) } else if enableExportV2 { - tagsMap := SplitTagsFlag(ctx.String(MetricsInfluxDBTagsFlag.Name)) + tagsMap := SplitTagsFlag(cfg.InfluxDBTags) log.Info("Enabling metrics export to InfluxDB (v2)") go influxdb.InfluxDBV2WithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, token, bucket, organization, "geth.", tagsMap) } - if ctx.IsSet(MetricsHTTPFlag.Name) { - address := net.JoinHostPort(ctx.String(MetricsHTTPFlag.Name), fmt.Sprintf("%d", ctx.Int(MetricsPortFlag.Name))) + if cfg.HTTP != "" { + address := net.JoinHostPort(cfg.HTTP, fmt.Sprintf("%d", cfg.Port)) log.Info("Enabling stand-alone metrics HTTP endpoint", "address", address) exp.Setup(address) - } else if ctx.IsSet(MetricsPortFlag.Name) { + } else if cfg.HTTP == "" && cfg.Port != 0 { log.Warn(fmt.Sprintf("--%s specified without --%s, metrics server will not start.", MetricsPortFlag.Name, MetricsHTTPFlag.Name)) } } From f6971a729c78c0b60aa95b3e96fd8c46ab6d2755 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Fri, 22 Nov 2024 08:43:16 +0100 Subject: [PATCH 2/4] cmd/utils: unindent --- cmd/utils/flags.go | 89 +++++++++++++++++++++++----------------------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 002ab647f1ed..2def98a3237e 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -1970,58 +1970,59 @@ func RegisterFullSyncTester(stack *node.Node, eth *eth.Ethereum, target common.H } func SetupMetrics(cfg *metrics.Config) { - if cfg.Enabled { - log.Info("Enabling metrics collection") - var ( - enableExport = cfg.EnableInfluxDB - enableExportV2 = cfg.EnableInfluxDBV2 - ) - if enableExport && enableExportV2 { - Fatalf("Flags %v can't be used at the same time", strings.Join([]string{MetricsEnableInfluxDBFlag.Name, MetricsEnableInfluxDBV2Flag.Name}, ", ")) - } - if enableExport || enableExportV2 { - v1FlagIsSet := cfg.InfluxDBUsername != "" || cfg.InfluxDBPassword != "" - v2FlagIsSet := cfg.InfluxDBToken != "" || cfg.InfluxDBOrganization != "" || cfg.InfluxDBBucket != "" - - if enableExport && v2FlagIsSet { - Fatalf("Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2") - } else if enableExportV2 && v1FlagIsSet { - Fatalf("Flags --influxdb.metrics.username, --influxdb.metrics.password are only available for influxdb-v1") - } + if !cfg.Enabled { + return + } + log.Info("Enabling metrics collection") + var ( + enableExport = cfg.EnableInfluxDB + enableExportV2 = cfg.EnableInfluxDBV2 + ) + if enableExport && enableExportV2 { + Fatalf("Flags %v can't be used at the same time", strings.Join([]string{MetricsEnableInfluxDBFlag.Name, MetricsEnableInfluxDBV2Flag.Name}, ", ")) + } + if enableExport || enableExportV2 { + v1FlagIsSet := cfg.InfluxDBUsername != "" || cfg.InfluxDBPassword != "" + v2FlagIsSet := cfg.InfluxDBToken != "" || cfg.InfluxDBOrganization != "" || cfg.InfluxDBBucket != "" + + if enableExport && v2FlagIsSet { + Fatalf("Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2") + } else if enableExportV2 && v1FlagIsSet { + Fatalf("Flags --influxdb.metrics.username, --influxdb.metrics.password are only available for influxdb-v1") } + } - var ( - endpoint = cfg.InfluxDBEndpoint - database = cfg.InfluxDBDatabase - username = cfg.InfluxDBUsername - password = cfg.InfluxDBPassword - - token = cfg.InfluxDBToken - bucket = cfg.InfluxDBBucket - organization = cfg.InfluxDBOrganization - ) + var ( + endpoint = cfg.InfluxDBEndpoint + database = cfg.InfluxDBDatabase + username = cfg.InfluxDBUsername + password = cfg.InfluxDBPassword + + token = cfg.InfluxDBToken + bucket = cfg.InfluxDBBucket + organization = cfg.InfluxDBOrganization + ) - if enableExport { - tagsMap := SplitTagsFlag(cfg.InfluxDBTags) + if enableExport { + tagsMap := SplitTagsFlag(cfg.InfluxDBTags) - log.Info("Enabling metrics export to InfluxDB") + log.Info("Enabling metrics export to InfluxDB") - go influxdb.InfluxDBWithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, database, username, password, "geth.", tagsMap) - } else if enableExportV2 { - tagsMap := SplitTagsFlag(cfg.InfluxDBTags) + go influxdb.InfluxDBWithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, database, username, password, "geth.", tagsMap) + } else if enableExportV2 { + tagsMap := SplitTagsFlag(cfg.InfluxDBTags) - log.Info("Enabling metrics export to InfluxDB (v2)") + log.Info("Enabling metrics export to InfluxDB (v2)") - go influxdb.InfluxDBV2WithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, token, bucket, organization, "geth.", tagsMap) - } + go influxdb.InfluxDBV2WithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, token, bucket, organization, "geth.", tagsMap) + } - if cfg.HTTP != "" { - address := net.JoinHostPort(cfg.HTTP, fmt.Sprintf("%d", cfg.Port)) - log.Info("Enabling stand-alone metrics HTTP endpoint", "address", address) - exp.Setup(address) - } else if cfg.HTTP == "" && cfg.Port != 0 { - log.Warn(fmt.Sprintf("--%s specified without --%s, metrics server will not start.", MetricsPortFlag.Name, MetricsHTTPFlag.Name)) - } + if cfg.HTTP != "" { + address := net.JoinHostPort(cfg.HTTP, fmt.Sprintf("%d", cfg.Port)) + log.Info("Enabling stand-alone metrics HTTP endpoint", "address", address) + exp.Setup(address) + } else if cfg.HTTP == "" && cfg.Port != 0 { + log.Warn(fmt.Sprintf("--%s specified without --%s, metrics server will not start.", MetricsPortFlag.Name, MetricsHTTPFlag.Name)) } } From 1635da8021b242b5aab8faa02d5492d4a5d5c53e Mon Sep 17 00:00:00 2001 From: lightclient Date: Fri, 22 Nov 2024 06:47:25 -0700 Subject: [PATCH 3/4] metrics: peek into toml to decide enabling --- cmd/geth/config.go | 2 +- metrics/metrics.go | 67 +++++++++++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/cmd/geth/config.go b/cmd/geth/config.go index e1dc0fda311e..5f252ebf1d74 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -151,7 +151,7 @@ func loadBaseConfig(ctx *cli.Context) gethConfig { // Load config file. if file := ctx.String(configFileFlag.Name); file != "" { if err := loadConfig(file, &cfg); err != nil { - utils.Fatalf("%v", err) + utils.Fatalf("failed to load config: %v", err) } } diff --git a/metrics/metrics.go b/metrics/metrics.go index c7fe5c7333b9..f7b0c20e18f6 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -6,15 +6,16 @@ package metrics import ( + "flag" + "io" "os" "runtime/metrics" "runtime/pprof" "strconv" - "strings" "syscall" "time" - "github.com/ethereum/go-ethereum/log" + "github.com/naoina/toml" ) // Enabled is checked by the constructor functions for all of the @@ -24,34 +25,58 @@ import ( // for less cluttered pprof profiles. var Enabled = false -// enablerFlags is the CLI flag names to use to enable metrics collections. -var enablerFlags = []string{"metrics"} - -// enablerEnvVars is the env var names to use to enable metrics collections. -var enablerEnvVars = []string{"GETH_METRICS"} - // init enables or disables the metrics system. Since we need this to run before // any other code gets to create meters and timers, we'll actually do an ugly hack // and peek into the command line args for the metrics flag. func init() { - for _, enabler := range enablerEnvVars { - if val, found := syscall.Getenv(enabler); found && !Enabled { - if enable, _ := strconv.ParseBool(val); enable { // ignore error, flag parser will choke on it later - log.Info("Enabling metrics collection") - Enabled = true - } + if val, found := syscall.Getenv("GETH_METRICS"); found && !Enabled { + if enable, _ := strconv.ParseBool(val); enable { // ignore error, flag parser will choke on it later + Enabled = true } } - for _, arg := range os.Args { - flag := strings.TrimLeft(arg, "-") - for _, enabler := range enablerFlags { - if !Enabled && flag == enabler { - log.Info("Enabling metrics collection") - Enabled = true - } + fs := flag.NewFlagSet("", flag.ContinueOnError) + fs.SetOutput(io.Discard) + fs.Bool("metrics", false, "") + fs.String("config", "", "") + + // The flag package will quit parsing immediately if it encounters a flag that + // was not declared to it ahead of time. We could be fancy and try to look + // through the args to find the ones we care about, but then we'd need to + // handle the various ways that flags can be defined -- which was the point of + // using the flags package in the first place! So instead, let's chop off the + // first element in the args list each time a parse fails. This way we will + // eventually parse every arg and get the ones we care about. + for i := range os.Args[1:] { + if err := fs.Parse(os.Args[i+1:]); err == nil { + break } } + + // Now visit the flags we defined which are present in the args and see if we + // should enable metrics. + fs.Visit(func(f *flag.Flag) { + switch f.Name { + case "metrics": + Enabled = true + case "config": + data, err := os.ReadFile(f.Value.String()) + if err != nil { + return + } + var cfg map[string]map[string]any + if err := toml.Unmarshal(data, &cfg); err != nil { + return + } + // A config file is definitely being used and was parsed correct. Let's + // try to peek inside and see if metrics are listed as enabled. + if m, ok := cfg["Metrics"]; ok { + if v, ok := m["Enabled"].(bool); ok && v { + Enabled = true + } + } + } + }) } var threadCreateProfile = pprof.Lookup("threadcreate") From 4abf2e5afdcc0f42bc0e36c715f171a3ffcdff35 Mon Sep 17 00:00:00 2001 From: lightclient Date: Mon, 25 Nov 2024 07:51:23 -0700 Subject: [PATCH 4/4] metrics: actually read flag value --- metrics/metrics.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/metrics/metrics.go b/metrics/metrics.go index f7b0c20e18f6..aba26308cb45 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -56,11 +56,16 @@ func init() { // Now visit the flags we defined which are present in the args and see if we // should enable metrics. fs.Visit(func(f *flag.Flag) { + g, ok := f.Value.(flag.Getter) + if !ok { + // Don't expect this to fail, but skip over just in case it does. + return + } switch f.Name { case "metrics": - Enabled = true + Enabled = g.Get().(bool) case "config": - data, err := os.ReadFile(f.Value.String()) + data, err := os.ReadFile(g.Get().(string)) if err != nil { return }