Skip to content

Improve logging #7

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve logging #7

wants to merge 2 commits into from

Conversation

vklachkov
Copy link
Contributor

@vklachkov vklachkov commented Feb 2, 2025

Этот PR заменяет eprintln! логи на стандартные tracing и log. По-умолчанию активировано логирование через tracing, но также существует опциональная возможность логировать через log для проекта PyChainql

@vklachkov vklachkov requested a review from CertainLach February 2, 2025 17:08
#[cfg(all(feature = "log", feature = "log-tracing"))]
compile_error!("Features `log-tracing` and `log` cannot be enabled together");

#[cfg(all(not(feature = "log"), not(feature = "log-tracing")))]
Copy link
Member

Choose a reason for hiding this comment

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

Both log and tracing have forwarding facades, so both are interchangable if someone wants to use library other than we are.

Given that tracing became de-facto standard and just much better due to opentelemetry compatibility (Spans, log attributes, e.t.c), I think we should drop log support, and leave only tracing here.

#[cfg(all(feature = "log", not(feature = "log-tracing")))]
pub use log::{debug, info, warn, error};

pub const LOG_TARGET: &str = "chainql";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we necessarily need to have stable log target name (Honestly, polkadot-sdk in many cases too, it would've benefit from log granularity), the defaults (module name, e.g chainql_core::rebuild) are fine.

}

info!(target: LOG_TARGET, "preloaded {keys_count} keys ({left} left)");
Copy link
Member

Choose a reason for hiding this comment

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

If we really want to have nice output, we can use tracing spans here, then we can just use https://github.com/emersonford/tracing-indicatif for visualising span progress.

Leave it as-is for now.

@CertainLach
Copy link
Member

Also, there are conflicts with trunk

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