Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: accept &Span as arg in in_span #50

Closed
wants to merge 1 commit into from

Conversation

EAimTY
Copy link
Contributor

@EAimTY EAimTY commented Oct 17, 2024

Span::set_local_parent only takes &self, therefore it is possible to make FutureExt::in_span accept a &Span while still be able to take the ownership of a Span. This could make storing a Span more flexible.

This should be considered as a breaking change since a new generic is added to InSpan.

@andylokandy
Copy link
Collaborator

andylokandy commented Oct 18, 2024

In most cases, the futures wrapped by in_span will soon be passed to tokio::spawn() or something like that. A &Span will prevent it because it's not 'static.

@EAimTY
Copy link
Contributor Author

EAimTY commented Oct 19, 2024

The change did not break it - in_span now takes an impl Borrow<Span>, which can be either an owned Span or a &Span.

This does not break existing codebase using in_span(Span). Only in some extremely rare cases, e.g. using named InSpan<T> as field of a struct, need to be alterd to InSpan<T, Span> or InSpan<T,&'a Span>.

IMO this could enable many additional usage. For example:

  1. Reusing the span attched to a finished future
  2. Tracking Drop using stored span in a struct

@andylokandy
Copy link
Collaborator

I can not imagine the situation where in_span(&span) is necessary. Can you provide an example?

@EAimTY
Copy link
Contributor Author

EAimTY commented Oct 19, 2024

Here is an example:

struct Foo {
    span: Span,
}

impl Foo {
    async fn do_something(self: Arc<Self>) {
        async {
            // ...
        }
        .in_span(&self.span)
        .await;
    }

    async fn do_other_thing(self: Arc<Self>) {
        async {
            // ...
        }
        .in_span(&self.span)
        .await;
    }
}

async fn foo() {
    let foo_lifespan = Span::root("foo", todo!());
    let foo = Arc::new(Foo { span: foo_lifespan });

    // these 3 tasks are sharing the same span representing foo's lifespan, even in different threads
    tokio::spawn(foo.clone().do_something());
    tokio::spawn(foo.clone().do_other_thing());
    tokio::spawn(foo.clone().do_other_thing());
}

// sometimes a value's lifespan can be hard to track if it is wrapped in an `Arc`
// this could log the precise instant the value is dropped 
impl Drop for Foo {
    fn drop(&mut self) {
        Event::add_to_parent("dropped", &self.span, || []);
    }
}

@andylokandy
Copy link
Collaborator

This example may not work in real life:

struct Foo {
    span: Span,
}

impl Foo {
    async fn do_something(self: Arc<Self>) {
        async {
            // can not use `.await` here because &self.span is borrowed.
        }
        .in_span(&self.span)
        .await;
    }
}

Since the limitation exists, it's usually recommended to spawn a new span for the future:

struct Foo {
    span: Span,
}

impl Foo {
    async fn do_something(self: Arc<Self>) {
        async {
            // ...
        }
        .in_span(Span::enter_with_parent("do_something", &self.span))
        .await;
    }
}

@andylokandy
Copy link
Collaborator

Closing since no more discussion...

@andylokandy andylokandy closed this Nov 8, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants