Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce tracing #1

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open

Introduce tracing #1

wants to merge 27 commits into from

Conversation

Karrenbelt
Copy link
Owner

@Karrenbelt Karrenbelt commented Sep 4, 2024

and start to replace the log crate.

Update: so far in all but three directories of the repositories the log crate has been replaced with the tracing crate. The modules that remain are:

  1. features: because of features/tests/both_syntaxes_produce_correct_output.rs
  2. p2p: because of p2p/src/sync_manager.rs
  3. validator: because of validator/src/validator.rs

with the reason being that the log::log macro and log::Level enum are used, which do not have a direct equivalent on tracing. Noteworthy there is a self-implemented log macro as well

These will be addressed in a subsequent PR that will be opened on top of this base PR.

grandine/src/main.rs Outdated Show resolved Hide resolved
@@ -30,3 +34,28 @@ impl PeerLogMetrics {
.store(target_peer_count, Ordering::Relaxed)
}
}

pub fn setup_tracing() -> impl Drop {
let log_path = "logs/testing.log";
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: proper filename

Comment on lines -113 to +112
info!("Eth1 RPC URLs: [{}]", eth1_rpc_urls.iter().format(", "));
info!("Eth1 RPC URLs: {:?}", eth1_rpc_urls);
Copy link
Owner Author

@Karrenbelt Karrenbelt Sep 4, 2024

Choose a reason for hiding this comment

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

This change is required when switching from the log to the tracing crate, because otherwise a panic ensues

thread 'main' panicked at /home/zarathustra/.cargo/registry/src/index.crates.io-6f17d22bba15001f/itertools-0.13.0/src/format.rs:95:21:
Format: was already formatted once

This is important to note, because this is not a compilation but a runtime error, which entails that in case of this being an issue elsewhere - meaning where log has been exchanged for tracing - the invocation of any of the associated macro's may cause the program to panic.

This leads me to ponder the question how can best ensure this not occurring. There are cumbersome ways of achieving this, by writing individual tests for modules, or by gathering all log messages through the execution of the test suite (i.e. file, line, level, message) and ensuring that all those present in the code are found in the logs. A better approach seems to me to assess the test coverage (as in which lines are of the code are ran during execution of the test suite) and then making sure that all lines with tracing macro's have been covered; this would mean they must have logged and hence the test would have failed in case of a panic.

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.

2 participants