fix(node): make validator metrics address configurable via flag/env#1023
fix(node): make validator metrics address configurable via flag/env#1023curryxbo wants to merge 1 commit into
Conversation
PR #966 dropped the --metrics-hostname / --metrics-port flags (and their METRICS_HOSTNAME / METRICS_PORT env vars). Layer1-verify validators now bind the /metrics listener solely to the Tendermint [instrumentation] prometheus_listen_addr from config.toml, so operators who used to set a custom port via flag/env can no longer override it. Restore that override for the layer1 validator path: re-add the two flags (no default value, so unset means "not provided") and resolve the metrics address as explicit flag/env > config.toml prometheus_listen_addr. Default behavior is unchanged -- with neither flag set the node keeps using the config.toml address, and metrics stays on by default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughLayer1 verify mode now resolves the validator metrics address from optional CLI/environment overrides, with fallback to Tendermint configuration and a default port when only a hostname is provided. ChangesValidator metrics overrides
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI as CLI or environment
participant Resolver as resolveValidatorMetricsAddr
participant VerifyMode as Layer1 verify mode
participant MetricsServer as Validator metrics server
CLI->>Resolver: Provide metrics hostname and port
Resolver->>VerifyMode: Return override or fallback address
VerifyMode->>MetricsServer: Start server on resolved address
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/cmd/node/main.go`:
- Around line 419-434: Update resolveValidatorMetricsAddr to use ctx.IsSet for
metrics hostname and port, preserving the fallback only when neither option is
set and retaining an explicitly provided port value of zero. Apply
defaultMetricsPort only when the port is genuinely unset, then construct the
address with net.JoinHostPort so IPv6 hosts are bracketed correctly; convert the
port to the required string form using strconv.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa44c184-e866-45bc-9445-86ec7c0c4088
📒 Files selected for processing (2)
node/cmd/node/main.gonode/flags/flags.go
| // resolveValidatorMetricsAddr returns the address the layer1 validator metrics | ||
| // server binds to. The Tendermint [instrumentation] prometheus_listen_addr from | ||
| // config.toml is the default; an explicit --metrics-hostname / --metrics-port | ||
| // (or METRICS_HOSTNAME / METRICS_PORT) overrides it. The flags carry no default | ||
| // value, so an empty host and zero port mean "not set" and the fallback is kept. | ||
| func resolveValidatorMetricsAddr(ctx *cli.Context, fallback string) string { | ||
| host := ctx.GlobalString(flags.MetricsHostname.Name) | ||
| port := ctx.GlobalUint64(flags.MetricsPort.Name) | ||
| if host == "" && port == 0 { | ||
| return fallback | ||
| } | ||
| if port == 0 { | ||
| port = defaultMetricsPort | ||
| } | ||
| return fmt.Sprintf("%s:%d", host, port) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use net.JoinHostPort for safe address construction.
Concatenating the host and port with fmt.Sprintf("%s:%d", host, port) produces an invalid TCP address if an IPv6 hostname is provided (e.g., ::1:26660 instead of [::1]:26660), which will cause the metrics server to fail on startup. Use net.JoinHostPort to safely format the address.
Additionally, the PR description notes that unset options should remain distinguishable from explicit values. However, checking port == 0 cannot distinguish between an unset flag and an explicitly provided --metrics-port 0 (which gets incorrectly overwritten to 26660). If your version of urfave/cli reliably supports ctx.IsSet for environment variables, consider using it instead of zero-value checks to genuinely distinguish unset flags.
🛠️ Proposed fix
func resolveValidatorMetricsAddr(ctx *cli.Context, fallback string) string {
host := ctx.GlobalString(flags.MetricsHostname.Name)
port := ctx.GlobalUint64(flags.MetricsPort.Name)
if host == "" && port == 0 {
return fallback
}
if port == 0 {
port = defaultMetricsPort
}
- return fmt.Sprintf("%s:%d", host, port)
+ return net.JoinHostPort(host, strconv.FormatUint(port, 10))
}Ensure that "net" and "strconv" are imported at the top of the file if not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // resolveValidatorMetricsAddr returns the address the layer1 validator metrics | |
| // server binds to. The Tendermint [instrumentation] prometheus_listen_addr from | |
| // config.toml is the default; an explicit --metrics-hostname / --metrics-port | |
| // (or METRICS_HOSTNAME / METRICS_PORT) overrides it. The flags carry no default | |
| // value, so an empty host and zero port mean "not set" and the fallback is kept. | |
| func resolveValidatorMetricsAddr(ctx *cli.Context, fallback string) string { | |
| host := ctx.GlobalString(flags.MetricsHostname.Name) | |
| port := ctx.GlobalUint64(flags.MetricsPort.Name) | |
| if host == "" && port == 0 { | |
| return fallback | |
| } | |
| if port == 0 { | |
| port = defaultMetricsPort | |
| } | |
| return fmt.Sprintf("%s:%d", host, port) | |
| } | |
| // resolveValidatorMetricsAddr returns the address the layer1 validator metrics | |
| // server binds to. The Tendermint [instrumentation] prometheus_listen_addr from | |
| // config.toml is the default; an explicit --metrics-hostname / --metrics-port | |
| // (or METRICS_HOSTNAME / METRICS_PORT) overrides it. The flags carry no default | |
| // value, so an empty host and zero port mean "not set" and the fallback is kept. | |
| func resolveValidatorMetricsAddr(ctx *cli.Context, fallback string) string { | |
| host := ctx.GlobalString(flags.MetricsHostname.Name) | |
| port := ctx.GlobalUint64(flags.MetricsPort.Name) | |
| if host == "" && port == 0 { | |
| return fallback | |
| } | |
| if port == 0 { | |
| port = defaultMetricsPort | |
| } | |
| return net.JoinHostPort(host, strconv.FormatUint(port, 10)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/cmd/node/main.go` around lines 419 - 434, Update
resolveValidatorMetricsAddr to use ctx.IsSet for metrics hostname and port,
preserving the fallback only when neither option is set and retaining an
explicitly provided port value of zero. Apply defaultMetricsPort only when the
port is genuinely unset, then construct the address with net.JoinHostPort so
IPv6 hosts are bracketed correctly; convert the port to the required string form
using strconv.
1 participant
Problem
Before PR #966, a validator's metrics server was configurable through CLI flags / env vars:
--metrics-hostname/METRICS_HOSTNAME(default0.0.0.0)--metrics-port/METRICS_PORT(default26660)--metrics-server-enable/METRICS_SERVER_ENABLE#966 removed all three. Layer1-verify validators now start their
/metricslistener only from the Tendermint[instrumentation] prometheus_listen_addrinconfig.toml:So operators who set a custom metrics port via
--metrics-port/METRICS_PORTenv silently lost that override — the value is ignored and the node falls back to the default:26660. The only way to change it now is editingconfig.toml.Fix
Restore the flag/env override for the layer1 validator path only:
--metrics-hostname(METRICS_HOSTNAME) and--metrics-port(METRICS_PORT) — intentionally without default values, so an unset flag is distinguishable from an explicit one.config.tomlprometheus_listen_addr> tendermint default, via a smallresolveValidatorMetricsAddrhelper.Per discussion,
--metrics-server-enableis not restored — reintroducing it with its old default (false) would silently disable validator metrics. Metrics stays on by default; to disable, setprometheus_listen_addr = ""inconfig.toml(already handled bystartMetricsServer).Behavior / compatibility
config.tomlprometheus_listen_addr(default:26660).--metrics-port/METRICS_PORTset → bindshost:port(host defaults to all interfaces /26660if the other half is omitted)./metricsfrom the Tendermint listener as before.Test
go build ./cmd/... ./flags/...✅go vet ./cmd/node/✅🤖 Generated with Claude Code
Summary by CodeRabbit