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: add minitrace-futures to support instrumentation for futures #202

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

iGxnon
Copy link
Contributor

@iGxnon iGxnon commented Feb 4, 2024

Changes:

  • Add a crate minitrace-futures
  • Add a trait Instrumented
    • Provide in_span instrumentation.

Checklist:

  • Support futures 0.3.x
  • Support futures 0.1.x

Signed-off-by: iGxnon <igxnon@gmail.com>
@coveralls
Copy link

coveralls commented Feb 5, 2024

Pull Request Test Coverage Report for Build 7814940816

  • 0 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 78.666%

Totals Coverage Status
Change from base Build 7812064248: 0.01%
Covered Lines: 1722
Relevant Lines: 2189

💛 - Coveralls

@iGxnon
Copy link
Contributor Author

iGxnon commented Feb 5, 2024

How should I fill in the author field in the Cargo.toml file?

@andylokandy
Copy link
Collaborator

andylokandy commented Feb 5, 2024

I've a question about the maker: what is the proper way to use maker? For example, Sink has 4 methods to create span on poll, and the maker has no way to know which is called. And will a local parent available when get called?

@iGxnon
Copy link
Contributor Author

iGxnon commented Feb 5, 2024

Sink has 4 methods to create span on poll, and the maker has no way to know which is called.

Perhaps replace maker in enter_on_poll() with FnMut(&str) -> Span?

And will a local parent available when get called?

Can set_local_parent be left for the user to decide?

let mut drain = sink::drain().enter_on_poll(|| {
        let span = Span::enter_with_parent("SubTask" &root);
        // Set the local parent so that `LocalSpan`
        // becomes available in the sink.
        (span.set_local_parent(), span)
});

or

let mut drain = sink::drain().enter_on_poll(||  Span::enter_with_parent("SubTask" &root));

@andylokandy
Copy link
Collaborator

andylokandy commented Feb 6, 2024

This seems too complicated, and maybe we could break it down. Perhaps a use_root field to indicate EnterOnItem to create a root span for each item? But why use root here? Perhaps in_span and enter_on_poll can be removed since meaningless.

@iGxnon
Copy link
Contributor Author

iGxnon commented Feb 6, 2024

This seems too complicated, and maybe we could break it down

OK, let's confirm the approximate API first.

Perhaps a use_root field to indicate EnterOnItem to create a root span for each item?

It can be done with:

let s = stream! {
        for i in 0..2 {
            yield i;
        }
}
.enter_on_item(|| Span::root("Root", SpanContext::random()));

In my scenario, I want to create a span for the user from the end of the last request to the receipt of the next request data. I hope this span can also collect some log events from the codec layer (the span needs to be the local parent), and it is best to report them as early as possible (the span is a root span). And I found that the duration of this span can be used to calculate a user's request frequency. At last the span can serve as the parent span for the next root span, allowing users to trace from the socket layer to the application logic processing layer.

image

However, in fact, what I would prefer to do is to calculate the time consumed by the codec layer. It seems difficult to achieve without modifying the Item types of Stream and Sink.

If I want to fully customize the lifetime of the span, then need to change the type of the Item in Stream and Sink to the following:

enum Item<T> {
    StartSpan,
    EndSpan,
    Item(T),
}

The instrumentation layer opens a span upon receiving StartSpan and ends the span upon receiving EndSpan.

Perhaps in_span and enter_on_poll can be removed since meaningless.

For some simple streams or sinks, such as a snapshot stream that closes after complete reception, we can use in_span to handle it. The enter_on_poll is designed to provide an API similar to FutureExt and it could be removed.

@andylokandy
Copy link
Collaborator

andylokandy commented Feb 7, 2024

It can be done with:

let s = stream! {
        for i in 0..2 {
            yield i;
        }
}
.enter_on_item(|| Span::root("Root", SpanContext::random()));

I mean, instead of a flexible but error-prone maker, a specialized use_root is better, if the only thing we want is to switch between Span and root span.

However, in fact, what I would prefer to do is to calculate the time consumed by the codec layer. It seems difficult to achieve without modifying the Item types of Stream and Sink.

Maybe you can do it by implementing https://docs.rs/tokio-util/latest/tokio_util/codec/trait.Decoder.html?

Edit:

with #204, maybe no need for creating root span here.

@andylokandy
Copy link
Collaborator

tracing-futures will not create new span for items but only enter a span, which will last the entire lifetime of the steam/sink. It seems like in_span in minitrace.

Signed-off-by: iGxnon <igxnon@gmail.com>
@iGxnon
Copy link
Contributor Author

iGxnon commented Feb 7, 2024

with #204, maybe no need for creating root span here.

Thanks! This is indeed very helpful.

tracing-futures will not create new span for items but only enter a span, which will last the entire lifetime of the steam/sink. It seems like in_span in minitrace.

OK, I have pruned this pull request to only include the instrumentation for in_span. It is ready for review.

@iGxnon
Copy link
Contributor Author

iGxnon commented Feb 7, 2024

There is another question: is enter_on_item still needed? If so, I will submit a new PR to try to constrain the maker solution.

@andylokandy
Copy link
Collaborator

is enter_on_item still needed?

I believe it will prove valuable if we can precisely define the semantics of enter_on_item (specifically, determining the precise moment that is considered as enter_on_item) and also develop a concrete example or use case to illustrate its application.

@andylokandy andylokandy merged commit 9194409 into tikv:master Feb 7, 2024
9 checks passed
@andylokandy
Copy link
Collaborator

@iGxnon Thank you!

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.

3 participants