Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
another round of docs and a slip of the pen
  • Loading branch information
drahnr committed Mar 14, 2022
commit 81937b37bf4195ee0a4e9fb1de171212e9bbbae1
15 changes: 9 additions & 6 deletions node/gum/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,22 @@
### Context

For the cross referencing in grafana to work, an explicit `traceID` of type `u128` has to be provided, both in the log line and the `jaeger::Span`.
Before this PR, no such `traceID` annotations could be made easily accross the codebase.

Related:
Related issues:

* <https://github.com/paritytech/polkadot/issues/5045>

### Decision

The various options are to add ~3 lines per tracing line including a `candidate_hash` reference, to derive the `TraceIdentifier` from that.
This would have avoided the complexity introduced with this crate.
The visual overhead and friction and required diligence to keep this up is unreasonably high in the mid/long run especially given a certain growth of the company.
The various options are to add ~2 lines per tracing line including a `candidate_hash` reference, to derive the `TraceIdentifier` from that.
This would have avoided the complexity introduced by adding another proc-macro to the codebase.
The visual overhead and friction and required diligence to keep the 100s of `tracing::{warn!,info!,debug!,..}` up is unreasonably high in the mid/long run especially with the context of more people joining the team.

### Consequences

Minimal training is required to name `CandidateHash` as `candidate_hash` when providing to any of the log macros (`warn!`, `info!`, etc.).
Minimal training/impact is required to name `CandidateHash` as `candidate_hash` when providing to any of the log macros (`warn!`, `info!`, etc.).

The crate has to be used throughout the entire codebase to work consistently.
The crate has to be used throughout the entire codebase to work consistently, to disambiguate, the prefix `gum::` is used.

Feature parity with `tracing::{warn!,..}` is not desired. We want consistency more than anything else. All currently used features _are_ supported with _gum_ as well.
18 changes: 15 additions & 3 deletions node/gum/proc-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

#![deny(unused_crate_dependencies)]
#![deny(missing_docs)]
#![deny(clippy::dbg_macro)]

//! Generative part of `tracing-gum`. See `tracing-gum` for usage documentation.

use proc_macro2::{Ident, Span, TokenStream};
use quote::{quote, ToTokens};
Expand All @@ -27,31 +31,37 @@ use self::types::*;
#[cfg(test)]
mod tests;

/// Print an error message.
#[proc_macro]
pub fn error(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
gum(item, Level::Error)
}

/// Print a warning level message.
#[proc_macro]
pub fn warn(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
gum(item, Level::Warn)
}

/// Print a info level message.
#[proc_macro]
pub fn info(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
gum(item, Level::Info)
}

/// Print a debug level message.
#[proc_macro]
pub fn debug(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
gum(item, Level::Debug)
}

/// Print a trace level message.
#[proc_macro]
pub fn trace(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
gum(item, Level::Trace)
}

/// One-size-fits all internal implementation that produces the actual code.
pub(crate) fn gum(item: proc_macro::TokenStream, level: Level) -> proc_macro::TokenStream {
let item: TokenStream = item.into();

Expand All @@ -66,6 +76,9 @@ pub(crate) fn gum(item: proc_macro::TokenStream, level: Level) -> proc_macro::To
res.unwrap_or_else(|err| err.to_compile_error()).into()
}

/// Does the actual parsing and token generation based on `proc_macro2` types.
///
/// Required for unit tests.
pub(crate) fn impl_gum2(orig: TokenStream, level: Level) -> Result<TokenStream> {
let args: Args = parse2(orig)?;

Expand All @@ -75,9 +88,7 @@ pub(crate) fn impl_gum2(orig: TokenStream, level: Level) -> Result<TokenStream>
let Args { target, comma, mut values, fmt } = args;

// find a value or alias called `candidate_hash`.
let maybe_candidate_hash = values.iter_mut().find(|value| {
value.as_ident() == "candidate_hash"
});
let maybe_candidate_hash = values.iter_mut().find(|value| value.as_ident() == "candidate_hash");
Copy link
Contributor

Choose a reason for hiding this comment

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

As a followup PR: Is it possible to remove the hardcoding here to derive the trace_id from something specified by the user ?

Copy link
Contributor Author

@drahnr drahnr Mar 15, 2022

Choose a reason for hiding this comment

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

Technically yes, but that would add additional syntax. Open to discuss something along the lines of foo @ traceID though, out of scope for this PR though


if let Some(kv) = maybe_candidate_hash {
let (ident, rhs_expr, replace_with) = match kv {
Expand Down Expand Up @@ -141,6 +152,7 @@ pub(crate) fn impl_gum2(orig: TokenStream, level: Level) -> Result<TokenStream>
}
}

/// Extract the support crate path.
fn support_crate() -> TokenStream {
let support_crate_name = if cfg!(test) {
quote! {crate}
Expand Down
2 changes: 1 addition & 1 deletion node/gum/proc-macro/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl Parse for ValueWithFormatMarker {

let lookahead = input.lookahead1();
let dot = if lookahead.peek(Token![.]) {
let dot = Some(dbg!(input.parse::<Token![.]>())?);
let dot = Some(input.parse::<Token![.]>()?);

loop {
let member = input.parse::<syn::Member>()?;
Expand Down
4 changes: 4 additions & 0 deletions node/gum/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

#![deny(unused_crate_dependencies)]
#![deny(missing_docs)]
#![deny(clippy::dbg_macro)]

//! A wrapper around `tracing` macros, to provide semi automatic
//! `traceID` annotation without codebase turnover.

Expand Down