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 #37

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

Conversation

Karrenbelt
Copy link

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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

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 we can best ensure this doesn't occur. 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 in case of a panic the test would not have passed.

Comment on lines +38 to +39
pub fn setup_tracing() -> impl Drop {
let log_path = "logs/testing.log";
Copy link
Author

Choose a reason for hiding this comment

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

TODO: proper filename

@Karrenbelt
Copy link
Author

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

I am not going to sign this. Please close my PR if this is a strict requirement and I will move on and spend my time elsewhere

@sauliusgrigaitis
Copy link
Member

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

I am not going to sign this. Please close my PR if this is a strict requirement and I will move on and spend my time elsewhere

I'm very interested why signing CLA is an issue. Are you against the fact that you need to sign anything (no matter what is it)? Or you are not happy that your contribution could be published under some other license too (we need this because we already hear that people would like to get Grandine licensed under some other licenses, so we may need to relicense in the future if critical mass is reached). In any case, your contribution will be always available under the current GPL license. This CLA is generated with https://contributoragreements.org/.

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.

3 participants