Skip to content

feat(o11y): Attach tracing context to messages received by PeerManagerActor - #7866

Merged
near-bulldozer[bot] merged 17 commits into
masterfrom
nikurt-pma-context
Oct 19, 2022
Merged

feat(o11y): Attach tracing context to messages received by PeerManagerActor#7866
near-bulldozer[bot] merged 17 commits into
masterfrom
nikurt-pma-context

Conversation

@nikurt

@nikurt nikurt commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

This PR complete tracing of async work across all actix Actors.
https://pagodaplatform.atlassian.net/browse/ND-171

Nikolay Kurtov and others added 12 commits October 18, 2022 17:20
There are two typos to be fixed.
1) `transction` needs to be fixed to `transaction`.
2) `valdiator` needs to be fixed to `validator`.
This does two rather big code motions at the same time (sorry!):

* precompilation code is moved from "let's match on VMKind" style to "let's call Runtime's virtual method". To do so, we re-purpose existing `precompile` functions in the trait. Remember, those fns are essentially dead code from "pipelined" compilation
* code to deal with our two-layered caching is moved from `cache.rs` into impls of VM
This way the client actors are abstracted out from the network crate.
Also moved the multiplexing PeerMessage -> (View)ClientActor API, from PeerActor to NetworkState.
Improved tests which started failing (this PR doesn't introduce semantic changes, therefore those tests were apparently too fragile).
@nikurt
nikurt marked this pull request as ready for review October 19, 2022 08:54
@nikurt
nikurt requested a review from a team as a code owner October 19, 2022 08:54
@nikurt
nikurt requested a review from mzhangmzz October 19, 2022 08:54
@nikurt
nikurt requested a review from nagisa October 19, 2022 09:14
Comment thread core/o11y/src/macros.rs
span.set_parent(context);
(span, msg)
}};
}

@nagisa nagisa Oct 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of duplicating the macro body, consider making handler_span able to take a parameter that describes level (and invokes tracing::span!) and then add handler_debug_span/handler_trace_span helpers.

The matcher will likely look a lot like this:

macro_rules! handler_span {
    (target: $target:expr, $lvl:expr, $msg:expr, $($extra_fields:tt)*) => {{
        let WithSpanContext { msg, context, .. } = $msg;
        let span = $crate::tracing::span!(
            target: $target,
            $lvl,
            "handle",
            handler = $crate::macros::type_name_of(&msg),
            actor = $crate::macros::last_component_of_name(std::any::type_name::<Self>()),
            $($extra_fields)*)
        .entered();
        span.set_parent(context);
        (span, msg)
    }}
}

macro_rules! handler_trace_span {
    ($target:expr, $msg:expr, $($extra_fields:tt)*) => {
        $crate::handler_span!(target: $target, $crate::tracing::Level::TRACE, $msg, $($extra_fields)*
    }
}

I see a couple issues I didn’t spot last time too:

  1. it is worth it to keep actor and handler expressions within the span! macro, as this way they will end up within the if ... {} condition that decides whether the span is enabled at all, and will be guaranteed to not be processed if the span is disabled;
  2. The extra_fields tt matcher should not be separated by commas, I think. It prevents passing through valid token forests such as foo = expression() etc.

@nikurt nikurt Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Followed the approach from https://docs.rs/tracing/latest/src/tracing/macros.rs.html#24 .
Tested that handler_trace_span! can be called with repeated extra fields, including a = 2+3 fields.

@nagisa nagisa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great!

@near-bulldozer
near-bulldozer Bot merged commit 4b01657 into master Oct 19, 2022
@near-bulldozer
near-bulldozer Bot deleted the nikurt-pma-context branch October 19, 2022 13:03
nikurt added a commit that referenced this pull request Nov 9, 2022
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.

5 participants