Allow overriding tracing filter with `RUST_LOG` env var #1896

romac · 2022-02-21T09:53:04Z

Closes: #1895

Usage

With this PR, one can set the RUST_LOG variable to tendermint_rpc=debug,ibc_relayer=info,ibc_relayer_cli=info in order to enable debug output for tendermint-rpc and info output for the relayer.

RUST_LOG=tendermint_rpc=debug,ibc_relayer=info,ibc_relayer_cli=info hermes start

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

adizere

Left 2 questions, nothing serious.

adizere · 2022-02-21T10:04:03Z

relayer-cli/src/components.rs

-        .unwrap();
+fn build_tracing_filter(log_level: LogLevel) -> Result<EnvFilter, FrameworkError> {
+    let directive = std::env::var(HERMES_LOG_VAR).unwrap_or_else(|_| {
+        let target_crates = ["ibc_relayer", "ibc_relayer_cli"];


I see here we're using RUST_LOG not only to replace the log level, but also the crates/modules. Can we give an example on how we're supposed to use RUST_LOG to overwrite the tracing parameters?

Yes, using RUST_LOG will override everything. I'll add an example, perhaps something like:

RUST_LOG=tendermint_rpc=debug,ibc_relayer=info,ibc_relayer_cli=info hermes start

adizere · 2022-02-21T10:05:37Z

relayer-cli/src/components.rs


 use crate::config::Error;

+/// The name of the environment variable through which one can override
+/// the tracing filter built in [`build_tracing_filter`].
+const HERMES_LOG_VAR: &str = "RUST_LOG";


I recall the integration tests framework making use of RUST_LOG, eg in cargo test. Would just like to flag that -- is there any interference between this approach and the pre-existing support for RUST_LOG in cargo?

The logs from the integration tests look fine to me: https://github.com/informalsystems/ibc-rs/runs/5272869928?check_suite_focus=true (modulo some doc tests failures, unrelated to this PR and best fixed elsewhere)

By the way, just to clarify, cargo test does not care itself about RUST_LOG but the integration tests themselves do, cf. https://github.com/informalsystems/ibc-rs/blob/8329d856844df30651f68a25ca5c5db4addbaa2b/tools/integration-test/src/bootstrap/single.rs#L75

What the test suite does is look at the RUST_LOG env variable, and then sets the log_level property in config.toml to that. If the test suite is only ever run with RUST_LOG=info/trace/debug/error/warn then everything should work as expected with no change in the output.

By the way, just to clarify, cargo test does not care itself about RUST_LOG but the integration tests themselves do, cf.

Thanks for clarifying that! This part wasn't obvious to me.

mzabaluev · 2022-02-21T10:09:59Z

relayer-cli/src/commands/start.rs

@@ -33,7 +33,7 @@ impl Runnable for StartCmd {
        let config = (*app_config()).clone();
        let config = Arc::new(RwLock::new(config));

-        let supervisor_handle = make_supervisor::<ProdChainHandle>(config.clone(), self.full_scan)
+        let supervisor_handle = make_supervisor::<ProdChainHandle>(config, self.full_scan)


Less cloning of massive data structures, good.

Looks like it was just cloning an Arc though, so it should have just consisted of incrementing a reference count.

It likely does not matter in the startup code, but in general coercing an Arc to a reference is still cheaper than atomically incrementing a ref count.

relayer-cli/src/components.rs

…`RUST_LOG` is set Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

adizere

Approved, but I still don't see any documentation on how to use RUST_LOG. Please remember to add that either in guide (release PR) or here.

adizere · 2022-02-21T13:03:21Z

relayer-cli/src/components.rs


 use crate::config::Error;

+/// The name of the environment variable through which one can override
+/// the tracing filter built in [`build_tracing_filter`].
+const HERMES_LOG_VAR: &str = "RUST_LOG";


By the way, just to clarify, cargo test does not care itself about RUST_LOG but the integration tests themselves do, cf.

Thanks for clarifying that! This part wasn't obvious to me.

…ems#1896) Closes: informalsystems#1895 With this PR, one can set the `RUST_LOG` variable to `tendermint_rpc=debug,ibc_relayer=info,ibc_relayer_cli=info` in order to enable debug output for `tendermint-rpc` and info output for the relayer. ```shell RUST_LOG=tendermint_rpc=debug,ibc_relayer=info,ibc_relayer_cli=info hermes start ``` --- * Allow overriding tracing filter with `RUST_LOG` env var * Fix warning * Add changelog entry * Rename `log_level` to `default_level` as the parameter is ignored if `RUST_LOG` is set Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems> * Better log message Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems> * Small refactor Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

romac added 3 commits February 21, 2022 10:50

Allow overriding tracing filter with RUST_LOG env var

e498e72

Fix warning

2239511

Add changelog entry

6cb07dd

romac requested review from adizere and ancazamfir as code owners February 21, 2022 09:53

adizere reviewed Feb 21, 2022

View reviewed changes

mzabaluev reviewed Feb 21, 2022

View reviewed changes

romac and others added 3 commits February 21, 2022 11:30

Rename log_level to default_level as the parameter is ignored if …

cfc8ea0

…`RUST_LOG` is set Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

Better log message

aef7a87

Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

Small refactor

8329d85

adizere approved these changes Feb 21, 2022

View reviewed changes

romac merged commit b6a4b06 into master Feb 21, 2022

romac deleted the romac/rust-log branch February 21, 2022 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding tracing filter with `RUST_LOG` env var #1896

Allow overriding tracing filter with `RUST_LOG` env var #1896

romac commented Feb 21, 2022 •

edited

Loading

adizere left a comment

adizere Feb 21, 2022

romac Feb 21, 2022

adizere Feb 21, 2022

romac Feb 21, 2022

romac Feb 21, 2022

romac Feb 21, 2022

adizere Feb 21, 2022

mzabaluev Feb 21, 2022

seanchen1991 Feb 21, 2022 •

edited

Loading

mzabaluev Feb 22, 2022 •

edited

Loading

adizere left a comment

adizere Feb 21, 2022

Allow overriding tracing filter with RUST_LOG env var #1896

Allow overriding tracing filter with RUST_LOG env var #1896

Conversation

romac commented Feb 21, 2022 • edited Loading

Usage

PR author checklist:

Reviewer checklist:

adizere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanchen1991 Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

mzabaluev Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow overriding tracing filter with `RUST_LOG` env var #1896

Allow overriding tracing filter with `RUST_LOG` env var #1896

romac commented Feb 21, 2022 •

edited

Loading

seanchen1991 Feb 21, 2022 •

edited

Loading

mzabaluev Feb 22, 2022 •

edited

Loading