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

Remove the additional virtual spans ThreadId(xxx)-{thread_name} for tracing-flame #3091

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

byte-sourcerer
Copy link

Motivation

tracing-flame generates a "virtual span" named ThreadId(xxx)-{thread_name} to identify the location of a thread within a span. When the time outside a span is large, the span of actual interest to users might appear exceedingly small.

The issue at hand is illustrated in the following example:

use std::thread::sleep;
use std::time::Duration;
use tracing::{span, Level};
use tracing_flame::FlameSubscriber;
use tracing_subscriber::{prelude::*, registry::Registry};

#[test]
fn capture_supported() {
    {
        let tmp_dir = tempfile::Builder::new()
            .prefix("tracing-flamegraph-test-")
            .tempdir()
            .expect("failed to create tempdir");
        let (flame_layer, _guard) = FlameSubscriber::with_file("./tracing.folded").unwrap();

        let subscriber = Registry::default().with(flame_layer);

        tracing::collect::set_global_default(subscriber).expect("Could not set global default");

        // long sleep!!!!
        sleep(Duration::from_millis(5000));

        {
            let span = span!(Level::ERROR, "outer");
            let _guard = span.enter();
            sleep(Duration::from_millis(10));
        }

        tmp_dir.close().expect("failed to delete tempdir");
    }
}

Pay attention to the line commented as long sleep, where the code spends much longer outside the "outer" span, which is what users are primarily concerned with.

The flame-graph spawned from the previous code looks something like this:

CleanShot 2024-09-28 at 17 22 17@2x

As you can see, the outer span appears barely noticeable.

Solution

We could remove the additional "virtual span" that is deemed unnecessary. Consequently, the flame graph for the above example ought to look more like this:

CleanShot 2024-09-28 at 17 24 46@2x

@byte-sourcerer byte-sourcerer requested review from yaahc and a team as code owners September 28, 2024 09:28
@kaffarell
Copy link
Contributor

kaffarell commented Oct 29, 2024

Hmm I'm not a flamegraph expert at all, but reading the "documentation" (https://www.brendangregg.com/flamegraphs.html the section "Summary") it says:

The x-axis shows the stack profile population, sorted alphabetically (it is not the passage of time), and the y-axis shows stack depth, counting from zero at the bottom. Each rectangle represents a stack frame. The wider a frame is is, the more often it was present in the stacks.

So I think the current approach is fine, because the outer span is just not present as often in the stack as the 5s sleep.
Let me know if this a stupid take.

@byte-sourcerer
Copy link
Author

So I think the current approach is fine, because the outer span is just not present as often in the stack as the 5s sleep. Let me know if this a stupid take.

Users are primarily interested in the span they have marked, yet our present approach does not fully acknowledge this concern. Nonetheless, if a user has a particular interest in the "virtual span", they have the option to illustrate it with span!.

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.

2 participants