slawlor / ractor

Rust actor framework
MIT License
1.31k stars 68 forks source link

Get Actor references from SupervisionEvent #183

Closed marcusirgens closed 8 months ago

marcusirgens commented 8 months ago

Add convenience methods on SupervisionEvent to quickly get the actor the event refers to.

I find myself frequently writing pattern matchers for the SupervisionEvent type, such as for tracing or other general supervision event handling. Here is one example from the code base I'm working with:

impl MyActor {
    #[tracing::instrument(skip_all, fields(actor.id = tracing::field::Empty, actor.name_pretty = tracing::field::Empty))]
    async fn handle_my_specialized_supervisor_evt(
        &self,
        myself: ActorRef<MyMessage>,
        message: SupervisionEvent,
        state: &mut MyState,
    ) -> Result<(), ActorProcessingErr> {
        use SupervisionEvent::*;
        if let (ActorPanicked(who, _) | ActorTerminated(who, _, _) | ActorStarted(who)) = &message {
            tracing::Span::current()
                .record("actor.name_pretty", tracing::field::debug(who))
                .record("actor.id", tracing::field::display(who.get_id()));
        }

        todo!() /* actual message handling */
    }
}

Here's another non-instrumentation-related message (using if let would be "cleaner", I think this is a copy-paste from the default handle_supervision_evt implementation):

impl MyActor {
    fn is_buildctl_runner_evt(
        &self,
        state: &mut BuildctlBuilderState,
        message: &SupervisionEvent,
    ) -> bool {
        use SupervisionEvent::*;
        match message {
            ActorPanicked(who, _) | ActorTerminated(who, _, _) | ActorStarted(who) => {
                state.managed_buildctl_builders.get_by_id(who.get_id()).is_some()
            }
            _ => false,
        }
    }
}

These could be rewritten, respectively, like the following (ignoring the mess that is field formatting with the tracing package...)

impl MyActor {
    #[tracing::instrument(skip_all, fields(
        actor.id = message.actor_id().map(tracing::field::display), 
        actor.name_pretty = message.actor_cell().map(tracing::field::debug)
    ))]
    async fn handle_my_specialized_supervisor_evt(
        &self,
        myself: ActorRef<MyMessage>,
        message: SupervisionEvent,
        state: &mut MyState,
    ) -> Result<(), ActorProcessingErr> {
        todo!() /* actual message handling */
    }
}

and (now with less pattern matching, at least?)

impl MyActor {
    fn is_buildctl_runner_evt(
        &self,
        state: &mut BuildctlBuilderState,
        message: &SupervisionEvent,
    ) -> bool {
        message
            .actor_id()
            .and_then(|id| state.managed_buildctl_builders.get_by_id(id))
            .is_some()
    }
}
marcusirgens commented 8 months ago

As an afterthought, it might be confusing that the ProcessGroupChanged variant, which does contain a list of ActorCell, does not cause these methods to return anything. Could be resolved by naming them differently?

codecov[bot] commented 8 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (c10c28f) 79.63% compared to head (c920757) 79.71%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #183 +/- ## ========================================== + Coverage 79.63% 79.71% +0.07% ========================================== Files 50 50 Lines 9708 9764 +56 ========================================== + Hits 7731 7783 +52 - Misses 1977 1981 +4 ``` | [Files](https://app.codecov.io/gh/slawlor/ractor/pull/183?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor) | Coverage Δ | | |---|---|---| | [ractor/src/actor/messages.rs](https://app.codecov.io/gh/slawlor/ractor/pull/183?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yL3NyYy9hY3Rvci9tZXNzYWdlcy5ycw==) | `67.90% <100.00%> (+4.52%)` | :arrow_up: | | [ractor/src/actor/tests/mod.rs](https://app.codecov.io/gh/slawlor/ractor/pull/183?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yL3NyYy9hY3Rvci90ZXN0cy9tb2QucnM=) | `91.47% <89.13%> (-0.18%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/slawlor/ractor/pull/183/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

marcusirgens commented 8 months ago

Rebased on c10c28f11ddffeb475eee69948171ae45a9fd029.

slawlor commented 8 months ago

Ah looks like the docs need an update. Likely just a module path specification, but I'd like the ci clean before merging

marcusirgens commented 8 months ago

Great, ill look at it and have a patch ready tomorrow. 🎉 On 11 Nov 2023 at 20:59 +0100, Sean Lawlor @.***>, wrote:

Ah looks like the docs need an update. Likely just a module path specification, but I'd like the ci clean before merging — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

marcusirgens commented 8 months ago

Likely just a module path specification

Hopefully corrected now.

As an afterthought, it might be confusing that the ProcessGroupChanged variant, which does contain a list of ActorCell, does not cause these methods to return anything. Could be resolved by naming them differently?

I changed to "an Actor lifecycle event" to make it slightly more obvious that these refer to started/terminated/panicked-events, does that sound OK to you, @slawlor?