Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics, cmd/geth: change init-process of metrics #30814

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 27, 2024

Alternative to #30781 .
This PR modifies how the metrics library handles Enabled: previously, the package init decided whether to serve real metrics or just dummy-types.
This has several drawbacks:

  • During pkg init, we need to determine whether metrics are enabled or not. So we first hacked in a check if certain geth-specific commandline-flags were enabled. Then we added a similar check for geth-env-vars. Then we almost added a very elaborate check for toml-config-file, plus toml parsing.
  • Using "real" types and dummy types interchangeably means that everything is hidden behind interfaces. This has a performance penalty, and also it just adds a lot of code.

This PR removes the interface stuff, uses concrete types, and allows for the setting of Enabled to happen later. It is still assumed that metrics.Enable() is invoked fairly early on.

The somewhat 'heavy' operations, such as ticking meters and exp-decay, now checks the enable-flag to prevent resource leak.

The change may be large, but it's mostly pretty trivial, and from the last time I gutted the metrics, I ensured that we have fairly good test coverage.

@holiman
Copy link
Contributor Author

holiman commented Nov 27, 2024

Runtime examples

Load (and enable) from conf:

$ go run ./cmd/geth --metrics --metrics.addr 0.0.0.0 dumpconfig > conf && go run ./cmd/geth --metrics --config conf -dev 2>&1 | grep metrics
INFO [11-27|06:56:21.987] Maximum peer count                       ETH=50 total=50
INFO [11-27|06:56:22.028] Set global gas cap                       cap=50,000,000
INFO [11-27|06:56:22.029] Initializing the KZG library             backend=gokzg
INFO [11-27|06:56:25.473] Enabling metrics collection
INFO [11-27|06:56:25.473] Enabling stand-alone metrics HTTP endpoint address=0.0.0.0:6060
INFO [11-27|06:56:25.473] Starting metrics server                  addr=http://0.0.0.0:6060/debug/metrics

Load from conf, but override with commandline

$ 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-27|06:57:09.251] Maximum peer count                       ETH=50 total=50
INFO [11-27|06:57:09.273] Set global gas cap                       cap=50,000,000
INFO [11-27|06:57:09.274] Initializing the KZG library             backend=gokzg
INFO [11-27|06:57:12.972] Enabling metrics collection
INFO [11-27|06:57:12.972] Enabling stand-alone metrics HTTP endpoint address=1.1.1.1:6060
INFO [11-27|06:57:12.972] Starting metrics server                  addr=http://1.1.1.1:6060/debug/metrics
ERROR[11-27|06:57:12.973] Failure in running metrics server        err="listen tcp 1.1.1.1:6060: bind: cannot assign requested address"

Load from conf, override to disable metrics

[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-27|06:57:39.724] Maximum peer count                       ETH=50 total=50
INFO [11-27|06:57:39.741] Set global gas cap                       cap=50,000,000
INFO [11-27|06:57:39.742] Initializing the KZG library             backend=gokzg

value atomic.Int64
}
// Gauge holds an int64 value that can be set arbitrarily.
type Gauge atomic.Int64
Copy link
Contributor

@fjl fjl Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this was changed to be just an int64. It doesn't improve the implementation code (have to convert it at every use), and the internals are fully exposed.

It's OK to define a struct with only one field sometimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bit inconsistent they way it is right now:

// StandardCounter is the standard implementation of a Counter and uses the
// sync/atomic package to manage a single int64 value.
type StandardCounter atomic.Int64

vs

// StandardGauge is the standard implementation of a Gauge and uses the
// sync/atomic package to manage a single int64 value.
type StandardGauge struct {
	value atomic.Int64
}

So I decided to just make them identical

@holiman
Copy link
Contributor Author

holiman commented Nov 28, 2024

Starting this on both 05 and 06 now. My first attempt failed, the config-check which was using the config now hit upon the previously unused fields in the defaultconfig:

	InfluxDBEndpoint: "http://localhost:8086",
	InfluxDBDatabase: "geth",
	InfluxDBUsername: "test",
	InfluxDBPassword: "test",
	InfluxDBTags:     "host=localhost",

	// influxdbv2-specific flags
	EnableInfluxDBV2:     false,
	InfluxDBToken:        "test",
	InfluxDBBucket:       "geth",

And thus croaked on a Fatal: Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2. This is now fixed: conflicts are checked on the flags, not the final config.

From what I can tell at a glance, the charts seem to be working like before

Screenshot 2024-11-28 at 09-49-00 Dual Geth - Grafana

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants