cli: config --p2p-relay-loglevel into tracing filter - #555
cli: config --p2p-relay-loglevel into tracing filter#555PoulavBhowmick03 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Wires the relay command’s --p2p-relay-loglevel flag into the tracing EnvFilter so operators can reduce libp2p relay noise (e.g., suppress benign libp2p_relay warnings) without changing Pluto’s overall --log-level.
Changes:
- Add
relay_filter(base, relay_level)helper to composelibp2p_relay=<level>into the tracing filter string. - Use the composed filter when building the relay’s console tracing config.
- Add a small unit test covering
relay_filterbehavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let p2p_relay = self.relay.p2p_relay_log_level; | ||
|
|
||
| let log_config = build_console_tracing_config( | ||
| relay_filter(self.log.level.as_str(), &p2p_relay), | ||
| &self.log.color, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
crates/cli/src/commands/relay.rs:21
relay_levelis taken verbatim from a flag/env var; leading/trailing whitespace (common in env vars) can make the composedEnvFilterinvalid and silently fall back to the default filter. Trimming here avoids surprising no-op configurations.
fn relay_filter(base: &str, relay_level: &str) -> String {
if relay_level.is_empty() {
base.to_string()
} else {
format!("{base},libp2p_relay={relay_level}")
crates/cli/src/commands/run.rs:25
- This file (and the PR) now includes substantial new behavior beyond wiring
--p2p-relay-loglevel(simnet flags/mocks, feature-set plumbing changes, etc.). Consider splitting the PR or updating the PR title/description to reflect the expanded scope so reviewers can assess risk appropriately.
//! Not every accepted flag is honored yet. Correctness-affecting flags with no
//! implementation (beacon-node headers, VC TLS, a preferred consensus protocol,
//! synthetic block proposals) fail fast with a "not yet supported" error, while
//! the remaining observability/availability flags with no implementation
//! (debug/pprof address, OTLP, proc directory, fallback beacon endpoints) are
crates/cli/src/commands/run.rs:600
String::leak()permanently leaks heap allocations. SinceAppConfig/tests can build multipleTestnetConfigs in a single process (unit tests, fuzzing, long-lived tooling), this can accumulate unnecessarily. Prefer changing theNetworktype (and registry) to own these strings (e.g.,String/Box<str>/Arc<str>), or otherwise store the backing strings somewhere owned rather than leaking.
pluto_eth2util::network::Network {
chain_id: testnet.chain_id,
name: testnet.name.clone().leak(),
genesis_fork_version_hex: testnet.genesis_fork_version_hex.clone().leak(),
genesis_timestamp: testnet.genesis_timestamp,
crates/app/src/node/mod.rs:1121
- The error message says "share index {i}", but
ihere is the index of the validator/pubshare in thevalidatorslist (0-based), not the node's share index. This can mislead operators trying to fix missing-keystore issues; report the validator index (or pubshare) instead.
for (i, pubshare) in pubshares.iter().enumerate() {
signer.sign(pubshare, b"test signing").map_err(|e| {
AppError::Simnet(format!(
"simnet validator key missing for share index {i}: {e}"
))
8bfa822 to
622b539
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/cli/src/commands/relay.rs:105
self.relay.p2p_relay_log_levelis moved intop2p_relay, butself.relayis subsequently accessed for other fields (http_address,auto_p2p_key, etc.). This is a partial move and should not compile. Destructureself.relayinto local variables (moving all fields at once) before building the tracing config and relay-server config builder.
let p2p_relay = self.relay.p2p_relay_log_level;
let log_config = build_console_tracing_config(
relay_filter(self.log.level.as_str(), &p2p_relay),
&self.log.color,
crates/cli/src/commands/relay.rs:21
relay_filtercan produce a filter string that starts with a comma ifbaseis empty andrelay_levelis set (e.g.",libp2p_relay=warn"). That will failEnvFilter::from_str(...)parsing and silently fall back to the default filter, so the relay loglevel flag would have no effect in that scenario. Handle an emptybaseexplicitly to avoid generating an invalid directive list.
fn relay_filter(base: &str, relay_level: &str) -> String {
if relay_level.is_empty() {
base.to_string()
} else {
format!("{base},libp2p_relay={relay_level}")
}
}
7509cf4 to
e298f4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/cli/src/commands/relay.rs:23
relay_filterappends the raw--p2p-relay-loglevelstring into the EnvFilter. If this value contains whitespace or an invalid level,EnvFilter::from_strwill fail andpluto_tracing::initwill silently fall back to the default filter (info), unintentionally discarding the user’s--log-level/ custom directives. Consider trimming and validating the relay level before appending, and ignore (or warn about) invalid values so the base filter remains effective.
fn relay_filter(base: &str, relay_level: &str) -> String {
if relay_level.is_empty() {
base.to_string()
} else {
format!("{base},libp2p_relay={relay_level}")
|
Hey @emlautarom1 , if there is anything in this PR that I need to change or work on, or address any other issues, do let me know |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/cli/src/commands/relay.rs:23
relay_filterblindly appendsrelay_levelinto anEnvFilterstring. If an operator passes an invalid value (e.g. whitespace, typo, or something containing,),pluto_tracing::initwill fail to parse the whole filter and silently fall back to the defaultEnvFilter("info"), effectively ignoring the user’s base--log-level.
It’s safer to trim + validate relay_level and ignore it (keeping base) when it’s not a recognized level string.
if relay_level.is_empty() {
base.to_string()
} else {
format!("{base},libp2p_relay={relay_level}")
}
emlautarom1
left a comment
There was a problem hiding this comment.
Verified that the wiring does suppress the warning from #483 properly when set to error while leaving our own logs at info.
Added the following:
- Dropped
Config::libp2p_log_level: dead code. - One-line doc on
relay_filterrecording the prefix-match contract. - Replaced the tests with a check for correct behavior. Fails in more scenarios (in case of regressions).
For future work: invalid levels are swallowed silently, for example: --p2p-relay-loglevel=fatal resets the filter to info and discards --log-level, and a bad --log-level like warning silences all output. Pre-existing issue and applies to run/dkg too, so better fixed across all level flags in a follow-up PR.
Closes #483