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

cmd/geth: dump config for metrics #22083

Merged
merged 7 commits into from
Jan 18, 2021

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Dec 28, 2020

closes #22007

./geth --metrics --metrics.influxdb  dumpconfig

@MariusVanDerWijden
Copy link
Member Author

@lightclient @MysticRyuujin could you take a look if that works for you?

@lightclient
Copy link
Member

Was able to test this with the metrics configuration I am using and it works great. Thanks @MariusVanDerWijden!

@MariusVanDerWijden MariusVanDerWijden marked this pull request as ready for review December 31, 2020 22:32
@holiman
Copy link
Contributor

holiman commented Jan 5, 2021

@MariusVanDerWijden is this still WIP, or is it ready for merge? If the latter, please change the title

@holiman
Copy link
Contributor

holiman commented Jan 5, 2021

I don't get it. This does dump out the metrics config to toml ... but it doesn't read the metrics config from the toml... ?

@holiman
Copy link
Contributor

holiman commented Jan 5, 2021

I would expect these two commands to give the exact same output:

[user@work go-ethereum]$ ./build/bin/geth --metrics --metrics.influxdb --metrics.port=7070  --metrics.expensive dumpconfig 2>/dev/null | grep Metrics -A10
[Metrics]
Enabled = true
EnabledExpensive = true
HTTP = "127.0.0.1"
Port = 7070
EnableInfluxDB = true
InfluxDBEndpoint = "http://localhost:8086"
InfluxDBDatabase = "geth"
InfluxDBUsername = "test"
InfluxDBPassword = "test"
InfluxDBTags = "host=localhost"
[user@work go-ethereum]$ ./build/bin/geth --metrics --metrics.influxdb --metrics.port=7070  --metrics.expensive dumpconfig  2>/dev/null  > test.conf && ./build/bin/geth --config test.conf dumpconfig 2>/dev/null | grep Metrics -A10

@MariusVanDerWijden
Copy link
Member Author

@holiman It did read the configs from toml, but it did overwrite them every time^^ It works now

@MariusVanDerWijden MariusVanDerWijden changed the title WIP cmd/geth: dump config cmd/geth: dump config for metrics Jan 5, 2021
@holiman
Copy link
Contributor

holiman commented Jan 6, 2021

Almost there now, but CLI should take precedence, so this should spit out 7171, not 7070:

./build/bin/geth --metrics --metrics.port=7070 dumpconfig  2>/dev/null  > test.conf && ./build/bin/geth --metrics.port=7171 --config test.conf dumpconfig 2>/dev/null | grep Metrics -A10 | grep Port
Port = 7070

@MariusVanDerWijden
Copy link
Member Author

MariusVanDerWijden commented Jan 6, 2021

Hmm it takes precedence if you enable --metrics explicitly

./build/bin/geth --metrics --metrics.port=7070 dumpconfig  2>/dev/null  > test.conf && ./build/bin/geth --metrics --metrics.port=7171 --config test.conf dumpconfig 2>/dev/null | grep Metrics -A10 | grep Port
Port = 7171

But I'll change it

@MariusVanDerWijden
Copy link
Member Author

So the code is quite a bit worse now but I think I need it to convey the whole logic.

  • If --metrics are enabled it should read the default values
  • If the metrics are read from file, they should be overwritten by the flags

@holiman
Copy link
Contributor

holiman commented Jan 7, 2021

I don't quite understand why you give special treatment to the enabled flags. If I set geth --x.foo.bar=baz --x.foo.enabled=true dumpconfig > conf, and later do geth --config conf --x.foo.enabled=false, IMO the config that is produced should have the feature disabled but the foo.bar field set to baz.

So geth --config conf.toml --metrics=false dumpconfig > conf.toml should only ever flip a boolean in the conf.toml, not erase the entire section, which I think the current implementation does... ?

Am I missing something?

@MariusVanDerWijden
Copy link
Member Author

No you're right, I thought that we wouldn't want things like influxdbpassword written in the config if it was disabled, but you're right that it should be written out anyway. changed now.
Now the metric section is always dumped even if --metrics or --influxdb are disabled.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

One minor comment, but generally looks ok. Will test it a bit

@@ -115,12 +129,28 @@ func defaultNodeConfig() node.Config {
return cfg
}

func defaultMetricConfig(ctx *cli.Context) metricConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be metricsFromCliArgs.. When I saw defaultMetricConfig being used before, I intuitively though that it just initialized some defaults, and didn't grokk that it actually read the cli arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's the thing with this strange logic, we need to read the metrics twice.
First we need to read the "default" metrics and apply them s.th. if we don't override them in the load config, they are set to the default values.
Then, after reading the config, we need to read the metrics again (this time only the specially set metrics) to overwrite the config file configs with the configs from cli.

So basically we do:

  • read config from cli regardless of whether they are set (reads defaults)
  • read config from file (overwrites the default configs if applicable)
  • read only the set configs from cli (overwrites the file configs)

I'll change the naming!

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Works for me!

Node: defaultNodeConfig(),
Eth: eth.DefaultConfig,
Node: defaultNodeConfig(),
Metrics: metricsFromCliArgs(ctx),
Copy link
Contributor

@fjl fjl Jan 13, 2021

Choose a reason for hiding this comment

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

I don't understand why you need this function (metricsFromCliArgs) if you also have applyMetricConfig. The idea behind the config file loading is:

The config is always created using the Go code defaults.
Then, the TOML file is loaded to override some things.
Finally, the flags are applied to override more things.

In this PR, for metrics, it takes the defaults from flags, then overrides with config file, then applies the flags again. I think we should remove metricsFromCliArgs and just define the defaults for metrics directly in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the problem with this approach is that we have now two different places where the default value of the flags is defined.
Once here and once in the cli.Flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way we usually solve this is by taking the flag default from the Go struct default.

https://github.com/ethereum/go-ethereum/blob/master/cmd/utils/flags.go#L136

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a new default var in the metrics package

@holiman holiman merged commit 10555d4 into ethereum:master Jan 18, 2021
@holiman holiman added this to the 1.10.0 milestone Jan 18, 2021
bulgakovk pushed a commit to bulgakovk/go-ethereum that referenced this pull request Jan 26, 2021
* cmd/geth: dump config

* cmd/geth: dump config

* cmd/geth: properly read config again

* cmd/geth: override metrics if flags are set

* cmd/geth: write metrics regardless if enabled

* cmd/geth: renamed to metricsfromcliargs

* metrics: add default configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dumpconfig doesn't output config for metrics
5 participants