ethereum / go-ethereum

Go implementation of the Ethereum protocol
https://geth.ethereum.org
GNU Lesser General Public License v3.0
47.71k stars 20.19k forks source link

cmd/geth: actually enable metrics when passed via toml #30781

Open lightclient opened 4 days ago

lightclient commented 4 days ago

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 commented 4 days ago

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

:+1:

holiman commented 3 days ago

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 commented 3 days ago

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.

lightclient commented 3 days ago

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 commented 3 days ago

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 ;-)