fix(cli): install the tracing subscriber before flag validation - #671
Open
emlautarom1-agent[bot] wants to merge 5 commits into
Open
fix(cli): install the tracing subscriber before flag validation#671emlautarom1-agent[bot] wants to merge 5 commits into
emlautarom1-agent[bot] wants to merge 5 commits into
Conversation
Consolidate the per-command log and Loki flags into a single `TracingArgs` flattened onto the root command as global args, and initialize tracing once in `main` from those flags, before any command builds its config. Warnings raised while validating flags — notably the insecure-relay warning — reach the subscriber on every command, and `enr`/`version` get a subscriber too. Console output goes to stderr so a command's stdout stays pipeable, and `--log-level` accepts only the levels Charon documents, so a typo cannot parse as an env-filter target directive and silently disable logging. `pluto_tracing::init` spawns the Loki background task and returns the worker, so the drain lifetime is owned in one place. Closes #604
Charon resolves the level through `zapcore.ParseLevel` and lowercases the color before matching, so both accept `INFO` and `FORCE` as readily as `info` and `force` — a common shape for values supplied through `CHARON_LOG_LEVEL` in unit files and compose manifests. Values that name no level at all stay rejected, so a typo cannot parse as an env-filter target directive and silently disable logging.
`main` reports a tracing-init failure and a clap parse failure directly on stderr, since neither has a subscriber to report through, so nothing converts either error into a `CliError`. Removing the variants also removes the `From` impls that made them look reachable.
![emlautarom1-agent[bot]](https://avatars.githubusercontent.com/u/31224949?s=60&v=4){kind=small}
Comment on lines
+67
to
+128
| #[arg( | ||
| long = "log-format", | ||
| env = "CHARON_LOG_FORMAT", | ||
| default_value = "console", | ||
| global = true, | ||
| display_order = 1000, | ||
| help = "Log format; console, logfmt or json" | ||
| )] | ||
| pub log_format: String, | ||
|
|
||
| #[arg( | ||
| long = "log-level", | ||
| env = "CHARON_LOG_LEVEL", | ||
| default_value = "info", | ||
| global = true, | ||
| ignore_case = true, | ||
| display_order = 1001, | ||
| help = "Log level; debug, info, warn or error" | ||
| )] | ||
| pub log_level: LogLevel, | ||
|
|
||
| #[arg( | ||
| long = "log-color", | ||
| env = "CHARON_LOG_COLOR", | ||
| default_value = "auto", | ||
| global = true, | ||
| ignore_case = true, | ||
| display_order = 1002, | ||
| help = "Log color; auto, force, disable." | ||
| )] | ||
| pub log_color: ConsoleColor, | ||
|
|
||
| #[arg( | ||
| long = "log-output-path", | ||
| env = "CHARON_LOG_OUTPUT_PATH", | ||
| global = true, | ||
| display_order = 1003, | ||
| help = "Path in which to write on-disk logs." | ||
| )] | ||
| pub log_output_path: Option<PathBuf>, | ||
|
|
||
| #[arg( | ||
| long = "loki-addresses", | ||
| env = "CHARON_LOKI_ADDRESSES", | ||
| value_delimiter = ',', | ||
| global = true, | ||
| display_order = 1004, | ||
| help = "Enables sending of logfmt structured logs to these Loki log aggregation server addresses. This is in addition to normal stderr logs." | ||
| )] | ||
| pub loki_addresses: Vec<String>, | ||
|
|
||
| #[arg( | ||
| long = "loki-service", | ||
| env = "CHARON_LOKI_SERVICE", | ||
| default_value = "pluto", | ||
| global = true, | ||
| display_order = 1005, | ||
| help = "Service label sent with logs to Loki." | ||
| )] | ||
| pub loki_service: String, | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Differs from Charon. All six flags are global, so they attach to every subcommand; Charon binds --log-* to run/relay/dkg and --loki-* to run/relay only.
With this, main can read the flags off the root before any subcommand exists, so no command can push in a conversion ahead of init. Scoping them per-command would put a match arm per command back into main, which is the coupling being removed.
| Err(err) => { | ||
| eprintln!("{err}"); | ||
| return ExitCode::FAILURE; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Why eprintln! survives here. These two early returns are the only paths with no subscriber to report through — the arg-match failure, and init itself failing. Every other error goes through error! so it also reaches Loki before the drain below.
`--log-format` and `--log-output-path` are accepted and never applied, and only the first `--loki-addresses` entry reaches a layer. A runtime warning covered one of the three and fired on every command that set it, including the ones that never read the flag. Note the gap on each field instead, so it is visible where the flag is declared and stays next to the work needed to close it.
emlautarom1
marked this pull request as ready for review
August 26, 2026 17:53
emlautarom1
approved these changes
Aug 26, 2026
Collaborator
There was a problem hiding this comment.
Required quite a bit of steering; final code is shorter due to the removal of duplication. Main changes are:
- Tracing flags are global and processed as soon as possible, so further parsing can signal using the tracing module.
- Replaced free standing strings with closed enums (ex. log levels)
- Made flags case insensitive to match Charon
- Logs go to stderr
- Removed some warnings due to unused flags; prefer to use TODOs instead.
iamquang95
approved these changes
Aug 27, 2026
| // no subscriber is installed yet (init happens later inside | ||
| // `commands::relay::run`), so write directly to stderr. | ||
| eprintln!( | ||
| "warning: {extra} additional --loki-addresses ignored; only the first is used", |
Collaborator
There was a problem hiding this comment.
I think we should keep this warning
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #604
Summary
pluto_tracing::initwas called from five places, each of them after the command had already converted and validated its own arguments, so anything raised during that conversion went to a subscriber that did not exist yet.--p2p-relays=http://…produced no insecure-relay warning at all onrun,unsafe run,dkgorrelay, andenr/versioninstalled no subscriber whatsoever.The log and Loki flags now live in one
TracingArgs, flattened onto the root command as clap globals.mainbuilds a singleTracingConfigfrom it and callspluto_tracing::initonce, before dispatching to any command. Since the flags no longer belong to the subcommand, no command can run its conversion ahead of init: the ordering is structural rather than a convention.initalso spawns the Loki background task and returns aLokiWorker, so the drain is owned in one place under a single flush budget. Centralizing both removes the per-command flag copies, their duplicated spawn and drain blocks, and the config fields that carried aTracingConfigno crate ever read.Differences from Charon
--log-*torun,relayanddkg, and--loki-*torunandrelay. Here all six attach to every subcommand, which is what letsmainread them off the root before any subcommand exists. Scoping them per-command would put a match arm per command back intomain— the coupling this removes. Consequence:dkggains the Loki flags, andenr,version,create *andalpha test *gain all six.--log-color=autokeys offNO_COLORrather than TTY detection. Charon usesterm.IsTerminal(os.Stderr.Fd()). The behaviour itself is unchanged here; it is called out because the flag now applies to every command.