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

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

Merged
merged 17 commits into from
Oct 19, 2022

Conversation

nikurt
Copy link
Contributor

@nikurt nikurt commented Oct 18, 2022

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

nikurt 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
span.set_parent(context);
(span, msg)
}};
}
Copy link
Collaborator

@nagisa nagisa Oct 19, 2022

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.

Copy link
Contributor Author

@nikurt nikurt Oct 19, 2022

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.

Copy link
Collaborator

@nagisa nagisa left a comment

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