-
Notifications
You must be signed in to change notification settings - Fork 411
feat: add optional tracing instrumentation
#1995
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
base: master
Are you sure you want to change the base?
Conversation
95a9633 to
a69d87d
Compare
a69d87d to
0646619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
It's quite inconvenient to repeat #[cfg(feature = "tracing-logs")] for every line logged.
How about we introduce some macros to solve this. These macros need to be shared across the whole workspace. I propose introducing an internal crate to the workspace (i.e. named common). Crates can be marked internal by setting the package.publish flag in Cargo.toml.
// crates/common/src/lib.rs
#[cfg(feature = "log")]
pub use tracing;
#[macro_use]
mod macros;// crates/common/src/log.rs
#[cfg(feature = "log")]
#[macro_export]
macro_rules! log_trace {
($($tt:tt)*) => {
tracing::trace!($($tt)*);
};
}
#[cfg(not(feature = "log"))]
#[macro_export]
macro_rules! log_trace {
($($tt:tt)*) => {};
}
// Also have macros for debug, info, etc.// crates/{downstream_crate}/Cargo.toml
[dependencies]
bdk_common = { path = "../common"}
[features]
// Forward log feature.
log = ["bdk_common/log"]// crates/{downstream_crate}/src/lib.rs
#[macro_use]
extern crate bdk_common;
// Usage:
log_trace!("This will only log if `log` is enabled");I would also rename the feature flag from tracing-logs to just log. There are only two widely used logging libraries in the rust ecosystem (tracing and log) and they are both interoperable so we wouldn't need multiple "logging backends".
Let me know what you think.
85c6d2b to
65988f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Could you add some trace logs in the description during a usual execution flow?
| let mut request: FullScanRequest<K> = request.into(); | ||
| let start_time = request.start_time(); | ||
|
|
||
| let _span = log_span!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just discovered the instrument tracing macro, maybe is helpful in these cases.
65988f1 to
0f4a3b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cACK 0f4a3b1
It's looking good, I think nymius's suggestion of instrumenting the functions is a good idea.
I also wonder if it's useful using some sort of correlation id (e.g uuid) so it's possible to correlate all logs related to each specific call/request, e.g full_scan/sync call or electrum/esplora requests.
0f4a3b1 to
f4c9032
Compare
Resolves #1934.
Description
This PR refactors BDK’s tracing instrumentation by introducing a new internal crate,
bdk_common, and replacing the oldtracing-logsfeature with a workspace-widelogfeature:bdk_commonwith macroslog_trace!,log_debug!,log_info!,log_warn!,log_error!, andlog_span!log(disabled by default) pulls intracing = "0.1"for all chain-sync crates#[cfg(feature = "tracing-logs")] use tracing…andtrace!/debug!calls forlog_trace!/log_span!invocations with structured fields--no-default-features(i.e. nolog), all logging macros become no-ops andtracingis not includedCoverage remains broad—entry points like
full_scan,sync,mempool_at,next_block, etc., now use structured field records via the new macros.Notes to the reviewers
Example code on the
tracing_examplebranch:https://github.com/LagginTimes/bdk/tree/tracing_example
Sample trace excerpt:
Changelog notice
bdk_commoncrate with feature-gated logging macros.logfeature in all chain crates to enabletracing‑based instrumentation.Checklists
All Submissions:
New Features:
Bugfixes: