cmd/geth: actually enable metrics when passed via toml

[lightclient]

closes #28303, replaces #28101

--

When we added metrics to the config toml back in #22083, they didn't actually seem to get enabled when you start the client with the toml. That is fixed here by only starting the metrics after we have parsed the config for it.

You can verify it's working by running this snippet:

$ go run ./cmd/geth --metrics --metrics.addr 0.0.0.0 dumpconfig > conf && go run ./cmd/geth --config conf -dev 2>&1 | grep metrics
INFO [11-21|21:02:06.148] Enabling metrics collection
INFO [11-21|21:02:06.148] Enabling stand-alone metrics HTTP endpoint address=0.0.0.0:6060
INFO [11-21|21:02:06.148] Starting metrics server                  addr=http://0.0.0.0:6060/debug/metrics

[holiman]

Some additional tests

^C[user@work go-ethereum]$ go run ./cmd/geth --metrics --metrics.addr 0.0.0.0 dumpconfig > conf && go run ./cmd/geth --config conf -dev --metrics.addr 1.1.1.1 2>&1 | grep metrics
INFO [11-21|14:46:47.851] Maximum peer count                       ETH=50 total=50
INFO [11-21|14:46:47.873] Set global gas cap                       cap=50,000,000
INFO [11-21|14:46:47.873] Initializing the KZG library             backend=gokzg
INFO [11-21|14:46:51.231] Enabling metrics collection
INFO [11-21|14:46:51.231] Enabling stand-alone metrics HTTP endpoint address=1.1.1.1:6060
INFO [11-21|14:46:51.231] Starting metrics server                  addr=http://1.1.1.1:6060/debug/metrics
ERROR[11-21|14:46:51.232] Failure in running metrics server        err="listen tcp 1.1.1.1:6060: bind: cannot assign requested address"
^C

[user@work go-ethereum]$ go run ./cmd/geth --metrics --metrics.addr 0.0.0.0 dumpconfig > conf && go run ./cmd/geth --config conf -dev --metrics=false --metrics.addr 1.1.1.1 2>&1 | grep metrics
INFO [11-21|14:47:09.190] Maximum peer count                       ETH=50 total=50
INFO [11-21|14:47:09.211] Set global gas cap                       cap=50,000,000
INFO [11-21|14:47:09.211] Initializing the KZG library             backend=gokzg

👍

[holiman]

LGTM

[holiman]

Actually, this doesn't quite work, as is. Check this:

diff --git a/metrics/metrics.go b/metrics/metrics.go
index c7fe5c7333..dae5217653 100644
--- a/metrics/metrics.go
+++ b/metrics/metrics.go
@@ -129,6 +129,7 @@ func readRuntimeStats(v *runtimeStats) {
 func CollectProcessMetrics(refresh time.Duration) {
        // Short circuit if the metrics system is disabled
        if !Enabled {
+               log.Info("Nah I don't wanna do metrics")
                return
        }

[user@work go-ethereum]$ go run ./cmd/geth --metrics --metrics.addr 0.0.0.0 dumpconfig > conf && go run ./cmd/geth --config conf -dev  2>&1 | grep metrics
INFO [11-22|08:46:03.217] Maximum peer count                       ETH=50 total=50
INFO [11-22|08:46:03.234] Set global gas cap                       cap=50,000,000
INFO [11-22|08:46:03.234] Initializing the KZG library             backend=gokzg
INFO [11-22|08:46:06.278] Enabling metrics collection
INFO [11-22|08:46:06.278] Enabling stand-alone metrics HTTP endpoint address=0.0.0.0:6060
INFO [11-22|08:46:06.278] Starting metrics server                  addr=http://0.0.0.0:6060/debug/metrics
INFO [11-22|08:46:06.278] Nah I don't wanna do metrics

The metrics.CollectProcessMetrics uses a package-global variable Enabled. It needs to be configured by external machinery.

[holiman]

So, as things work now, the --metrics flag must be set explicitly. Because that flag is parsed in the metrics package init, setting the global metrics.Enabled.

So, this PR does work in that it can set the metrics-parameters (creds, ports etc), but it cannot override the --metrics flag. So, we should detect if there's a mismatch and notify the user. Or somehow fix it so that we can enable metrics from config, but that seems difficult. Needs to be done early in the package load.

[holiman]

Sorry, computer says no

[lightclient]

Nice catch. What do you think about 3fe6879?

You can curl to see it is really putting out data:

curl http://0.0.0.0:6060/debug/metrics

Same hack that we already have except now we peek into the toml. This code path will execute very rarely and the toml lib is already a dependency. So seems reasonable?

[holiman]

Nice catch. What do you think about 3fe6879?

Hm. Yeah.. It's dirty but it works. I guess it's just another little slide, in the slope ;-)

[holiman]

Looks flaky...?

Let's assume metrics is enabled in toml config. Won't these two behave differently:

1. geth --metrics=false --conf=conf.toml will correctly set Enabled=false.
2. geth --conf=conf.toml --metrics=false will incorrectly set Enabled=true.

?

[lightclient]

So in this case, the order of precedence is always flags first, config second. However, the way init() is written in metrics/metrics.go, Enabled is always set to true in case either a flag or config specifies it should be. This is also how it was previously.

So for both 1) and 2) Enabled will be set to true, but we will not actually serve the metrics because that occurs later in the geth command start up. That is where the precedence comes in.

How would you prefer to deal with this? I could change the parsing in init() so that when the flag metrics is set, it always uses that value, regardless of config value. Otherwise could detect a mismatch (enabled in config, disabled explicitly by flag) during client startup and fail there instead.

Otherwise, we can leave as-is. Explicitly setting --metrics=false while actually passing a config with metrics enabled is pretty unlikely and we will not serve the metrics as expected. It will just cause geth to record the metrics internally.

I think my pref is to just fail when metrics are enabled in config, but explicitly disabled by the flag. It is simple to deal with:

diff --git a/cmd/geth/config.go b/cmd/geth/config.go
index 5f252ebf1..200b24109 100644
--- a/cmd/geth/config.go
+++ b/cmd/geth/config.go
@@ -289,6 +289,10 @@ func dumpConfig(ctx *cli.Context) error {

 func applyMetricConfig(ctx *cli.Context, cfg *gethConfig) {
        if ctx.IsSet(utils.MetricsEnabledFlag.Name) {
+               enabled := ctx.Bool(utils.MetricsEnabledFlag.Name)
+               if cfg.Metrics.Enabled && !enabled {
+                       utils.Fatalf("Flag --metrics mismatch with provided config.")
+               }
                cfg.Metrics.Enabled = ctx.Bool(utils.MetricsEnabledFlag.Name)
        }
        if ctx.IsSet(utils.MetricsEnabledExpensiveFlag.Name) {

What do you think?

[fjl]

@holiman will refactor package metrics to allow enabling at runtime.