Skip to content

feat(node): allow configuration of logging through configuration file#2500

Merged
DSharifi merged 7 commits intomainfrom
dsharifi/log-config-split
Mar 18, 2026
Merged

feat(node): allow configuration of logging through configuration file#2500
DSharifi merged 7 commits intomainfrom
dsharifi/log-config-split

Conversation

@DSharifi
Copy link
Copy Markdown
Contributor

@DSharifi DSharifi commented Mar 18, 2026

closes #2498

also closes #2501

@claude
Copy link
Copy Markdown

claude bot commented Mar 18, 2026

Code Review

Two issues found:

1. Missing #[serde(default)] on log_config field — backward compatibility break

StartConfig::log_config is a required field with no #[serde(default)] annotation. Any existing TOML config file used with start-with-config-file that doesn't have a [log_config] section will fail to deserialize with missing field log_config.

Fix: Add #[serde(default)] and implement Default for LogConfig:

// In StartConfig:
#[serde(default)]
pub log_config: LogConfig,

// New impl:
impl Default for LogConfig {
    fn default() -> Self {
        Self {
            log_level: None,
            log_format: LogFormat::Plain,
        }
    }
}

2. env_filter silently ignores RUST_LOG when log_level is set in config

When log_level is set in the config file, EnvFilter::new(level.as_str()) creates a simple global filter (e.g., "INFO"), completely ignoring any RUST_LOG environment variable. This means operators lose the ability to do per-module filtering (e.g., RUST_LOG=mpc_node=debug,hyper=warn) when a config-level log_level is set.

This may be intentional, but it's worth noting — consider documenting this precedence behavior, or using EnvFilter::builder().with_default_directive(level.into()).from_env_lossy() to allow RUST_LOG to override specific modules while using the config level as a baseline.

⚠️ Issue #1 should be addressed before merge.

@DSharifi
Copy link
Copy Markdown
Contributor Author

  1. Missing #[serde(default)] on log_config field — backward compatibility break

That's fine. Toml file is not used in prod yet.

  1. env_filter silently ignores RUST_LOG when log_level is set in config

That's intened. Env variables won't be able to be passed from launcher anyway in prod settings.

#[tokio::test(flavor = "multi_thread")]
#[test_log::test(tokio::test(flavor = "multi_thread"))]
async fn test_many_triple_generation() {
init_logging(LogFormat::Plain);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why did you remove it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See comment here
#2500 (comment)

netrome
netrome previously approved these changes Mar 18, 2026
Copy link
Copy Markdown
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Nice stuff, although I think the new variant loses some features of the existing env filter that would be nice to retain.

Comment thread crates/node/src/p2p.rs
use tokio::time::timeout;

#[tokio::test]
#[test_log::test]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why and when do we need these now? It doesn't seem to be on all tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See the diff for removed code. Previously we called init_logging (prod logggin) for tests. Since init_logging API changed I didn't want to litter all callsites with building a logconfig, instead reusing this crate which is good for logging/traces during tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah I see. I think some of these tests aren't testing the logs though and should run without it imo, but we can clean that up later.

Comment thread crates/node/src/tracing.rs Outdated
Comment on lines +21 to +24
fn env_filter(log_level: Option<Level>) -> EnvFilter {
match log_level {
Some(level) => EnvFilter::new(level.as_str()),
None => EnvFilter::from_default_env(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feels a bit weird to use an "env filter" when we're not using the env anymore.

Also, it's quite common with more advanced filters (like enabling debug for certain modules). It would be nice to support that with the config file as well, but that could be left as a follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's there to not break nodes that are not in tee, and most providers that don't use toml file for configuring which we just recently introducd.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to make it non optional in a follow up issue once only config file is supported for launch

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah makes sense.

barakeinav1
barakeinav1 previously approved these changes Mar 18, 2026
Copy link
Copy Markdown
Contributor

@barakeinav1 barakeinav1 left a comment

Choose a reason for hiding this comment

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

we are also losing the ability to set MPC for DEBUG while having the indexer as INFO.
approving, but we should have this as a follow-up issue.

@DSharifi DSharifi removed this pull request from the merge queue due to a manual request Mar 18, 2026
@DSharifi DSharifi dismissed stale reviews from barakeinav1 and netrome via ddac915 March 18, 2026 14:32
@DSharifi DSharifi enabled auto-merge March 18, 2026 14:44
Copy link
Copy Markdown
Contributor

@barakeinav1 barakeinav1 left a comment

Choose a reason for hiding this comment

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

  • Logging init is still deferred to run_mpc_node(), so config parsing errors before that point are unlogged. not sure it matters in practice.

approving

@DSharifi DSharifi added this pull request to the merge queue Mar 18, 2026
Merged via the queue into main with commit 546bef2 Mar 18, 2026
10 checks passed
@DSharifi DSharifi deleted the dsharifi/log-config-split branch March 18, 2026 15:51
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.

Support RUST_LOG-style per-crate log filtering in config file Add config-driven logging settings to mpc-node start config

3 participants