Add `tracing-flame` crate for generating flamegraphs/flamecharts #631

yaahc · 2020-03-11T16:33:42Z

No description provided.

tracing-flame/src/lib.rs

hawkw

this is awesome, thank you so much! i left some notes, hopefully they're helpful!

tracing-flame/Cargo.toml

tracing-flame/src/lib.rs

tracing-flame/Cargo.toml

tracing-flame/src/error.rs

tracing-flame/src/lib.rs

yaahc

One problem right now is that the test / example both create files in the current directory whenever run, I'm not sure how best to resolve this.

Co-Authored-By: Eliza Weisman <[email protected]> Co-Authored-By: David Barsky <[email protected]>

Co-Authored-By: David Barsky <[email protected]>

davidbarsky · 2020-04-02T15:31:31Z

@yaahc: I haven't tried this yet, but for:

One problem right now is that the test / example both create files in the current directory whenever run, I'm not sure how best to resolve this.

Would taking a development dependency on tempdir do the trick? I also know Rust Analyzer also makes use of a neat in-memory file system, so that could also resolve this problem.

tracing-flame/Cargo.toml

tracing-flame/tests/collapsed.rs

tracing-flame/src/lib.rs

hawkw

some docs nits. otherwise, if you think this API seems right, I'm good with it.

examples/examples/inferno_flame.rs

tracing-flame/src/error.rs

tracing-flame/src/lib.rs

Co-Authored-By: Eliza Weisman <[email protected]>

hawkw

I noticed a potential problem with the way we record the START timestamp. Besides that, I think this is looking good. I left a few more nits, and some docs edits, but I think this ought to be ready to merge fairly soon.

tracing-flame/src/lib.rs

hawkw · 2020-04-08T21:02:22Z

tracing-flame/src/lib.rs

+//! as they were emitted by `tracing-flame`, so that it is clear when each
+//! span is entered relative to others and get an accurate visual trace of
+//! the execution of your program. This representation is best created with a
+//! `flamechart`, which _does not_ sort or collapse identical stack frames.


This should probably end by explaining how to make a flamechart rather than a flamegraph?

We can't yet :S, its a feature I intend to add to inferno once this is released

ah, okay, nevermind then. it might be worth mentioning that so we don't incorrectly imply that you can make flamecharts in inferno?

tracing-flame/src/lib.rs

hawkw · 2020-04-08T21:04:08Z

tracing-flame/src/lib.rs

+where
+    S: Subscriber,
+    W: Write + 'static,


Is the where clause necessary here, or can these just be present on the impls for the type?

No, but I was under the impression that we'd get more useful compiler errors with the bounds on the type.

If users can't write out literals of the type, putting the bounds on the impl block with new in it should give them equally useful errors.

tracing-flame/src/lib.rs

tracing-flame/src/error.rs

tracing-flame/Cargo.toml

Co-Authored-By: Eliza Weisman <[email protected]>

hawkw · 2020-04-09T00:35:56Z

re: the start time thing, we should probably at least spot-check the output from a multithreaded program... we could take the existing example, spawn a couple threads that run the same code, and make sure the output isn't "totally weird"

hawkw

I noticed a panic on CI, and I think I know how to fix it.

Once that's fixed (and we have a test for multithreaded code), I think we can merge this.

hawkw · 2020-04-09T16:05:20Z

tracing-flame/Cargo.toml

+    "Eliza Weisman <[email protected]>",
+    "David Barsky <[email protected]>",


take it or leave it: i don't personally feel the need to be listed as an author for this crate...you did all the work :)

Same here; I just reviewed it :)

tracing-flame/Cargo.toml

tracing-flame/src/lib.rs

Co-Authored-By: Eliza Weisman <[email protected]>

davidbarsky

I feel pretty good about this, especially since we can update this later. No need to let this collect dust.

hawkw

A couple last non-blocking nits, but this looks pretty good to me. It would be good to add a README and stuff before publishing this, but that can be done in a follow-up branch.

tracing-flame/src/lib.rs

tracing-flame/tests/concurrent.rs

Motivation ---------- Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for customizing the io writer used by `fmt`. This trait is necessary (rather than simply using a `Write` instance) because the default implementation performs the IO on the thread where an event was recorded, meaning that a separate writer needs to be acquired by each thread (either by calling a function like `io::stdout`, by locking a shared `Write` instance, etc). Right now there is a blanket impl for `Fn() -> T where T: Write`. This works fine with functions like `io::stdout`. However, the _other_ common case for this trait is locking a shared writer. Therefore, it makes sense to see an implementation like this: ``` rust impl<'a, W: io::Write> MakeWriter for Mutex<W> where W: io::Write, { type Writer = MutexWriter<'a, W>; fn make_writer(&self) -> Self::Writer { MutexWriter(self.lock().unwrap()) } } pub struct MutexWriter<'a, W>(MutexGuard<'a, W>); impl<W: io::Write> io::Write for MutexWriter<'_, W> { // write to the shared writer in the `MutexGuard`... } ``` Unfortunately, it's impossible to write this. Since `MakeWriter` always takes an `&self` parameter and returns `Self::Writer`, the generic parameter is unbounded: ``` Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber) error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates --> tracing-subscriber/src/fmt/writer.rs:61:6 | 61 | impl<'a, W: io::Write> MakeWriter for Mutex<W> | ^^ unconstrained lifetime parameter error: aborting due to previous error ``` This essentially precludes any `MakeWriter` impl where the writer is borrowed from the type implementing `MakeWriter`. This is a significant blow to the usefulness of the trait. For example, it prevented the use of `MakeWriter` in `tracing-flame` as suggested in #631 (comment). Proposal -------- This PR changes `MakeWriter` to be generic over a lifetime `'a`: ```rust pub trait MakeWriter<'a> { type Writer: io::Write; fn make_writer(&'a self) -> Self::Writer; } ``` The `self` parameter is now borrowed for the `&'a` lifetime, so it is okay to return a writer borrowed from `self`, such as in the `Mutex` case. I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`. Unfortunately, this is a breaking change and will need to wait until we release `tracing-subscriber` 0.3. Fixes #675. Signed-off-by: Eliza Weisman <[email protected]>

## Motivation Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for customizing the io writer used by `fmt`. This trait is necessary (rather than simply using a `Write` instance) because the default implementation performs the IO on the thread where an event was recorded, meaning that a separate writer needs to be acquired by each thread (either by calling a function like `io::stdout`, by locking a shared `Write` instance, etc). Right now there is a blanket impl for `Fn() -> T where T: Write`. This works fine with functions like `io::stdout`. However, the _other_ common case for this trait is locking a shared writer. Therefore, it makes sense to see an implementation like this: ``` rust impl<'a, W: io::Write> MakeWriter for Mutex<W> where W: io::Write, { type Writer = MutexWriter<'a, W>; fn make_writer(&self) -> Self::Writer { MutexWriter(self.lock().unwrap()) } } pub struct MutexWriter<'a, W>(MutexGuard<'a, W>); impl<W: io::Write> io::Write for MutexWriter<'_, W> { // write to the shared writer in the `MutexGuard`... } ``` Unfortunately, it's impossible to write this. Since `MakeWriter` always takes an `&self` parameter and returns `Self::Writer`, the generic parameter is unbounded: ``` Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber) error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates --> tracing-subscriber/src/fmt/writer.rs:61:6 | 61 | impl<'a, W: io::Write> MakeWriter for Mutex<W> | ^^ unconstrained lifetime parameter error: aborting due to previous error ``` This essentially precludes any `MakeWriter` impl where the writer is borrowed from the type implementing `MakeWriter`. This is a significant blow to the usefulness of the trait. For example, it prevented the use of `MakeWriter` in `tracing-flame` as suggested in #631 (comment). ## Proposal This PR changes `MakeWriter` to be generic over a lifetime `'a`: ```rust pub trait MakeWriter<'a> { type Writer: io::Write; fn make_writer(&'a self) -> Self::Writer; } ``` The `self` parameter is now borrowed for the `&'a` lifetime, so it is okay to return a writer borrowed from `self`, such as in the `Mutex` case. I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`. Unfortunately, this is a breaking change and will need to wait until we release `tracing-subscriber` 0.3. Fixes #675. Signed-off-by: Eliza Weisman <[email protected]>

This backports PR #781 from `master`. ## Motivation Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for customizing the io writer used by `fmt`. This trait is necessary (rather than simply using a `Write` instance) because the default implementation performs the IO on the thread where an event was recorded, meaning that a separate writer needs to be acquired by each thread (either by calling a function like `io::stdout`, by locking a shared `Write` instance, etc). Right now there is a blanket impl for `Fn() -> T where T: Write`. This works fine with functions like `io::stdout`. However, the _other_ common case for this trait is locking a shared writer. Therefore, it makes sense to see an implementation like this: ``` rust impl<'a, W: io::Write> MakeWriter for Mutex<W> where W: io::Write, { type Writer = MutexWriter<'a, W>; fn make_writer(&self) -> Self::Writer { MutexWriter(self.lock().unwrap()) } } pub struct MutexWriter<'a, W>(MutexGuard<'a, W>); impl<W: io::Write> io::Write for MutexWriter<'_, W> { // write to the shared writer in the `MutexGuard`... } ``` Unfortunately, it's impossible to write this. Since `MakeWriter` always takes an `&self` parameter and returns `Self::Writer`, the generic parameter is unbounded: ``` Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber) error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates --> tracing-subscriber/src/fmt/writer.rs:61:6 | 61 | impl<'a, W: io::Write> MakeWriter for Mutex<W> | ^^ unconstrained lifetime parameter error: aborting due to previous error ``` This essentially precludes any `MakeWriter` impl where the writer is borrowed from the type implementing `MakeWriter`. This is a significant blow to the usefulness of the trait. For example, it prevented the use of `MakeWriter` in `tracing-flame` as suggested in #631 (comment). ## Proposal This PR changes `MakeWriter` to be generic over a lifetime `'a`: ```rust pub trait MakeWriter<'a> { type Writer: io::Write; fn make_writer(&'a self) -> Self::Writer; } ``` The `self` parameter is now borrowed for the `&'a` lifetime, so it is okay to return a writer borrowed from `self`, such as in the `Mutex` case. I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`. Unfortunately, this is a breaking change and will need to wait until we release `tracing-subscriber` 0.3. Fixes #675. Signed-off-by: Eliza Weisman <[email protected]>

…kio-rs#1654) This backports PR tokio-rs#781 from `master`. ## Motivation Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for customizing the io writer used by `fmt`. This trait is necessary (rather than simply using a `Write` instance) because the default implementation performs the IO on the thread where an event was recorded, meaning that a separate writer needs to be acquired by each thread (either by calling a function like `io::stdout`, by locking a shared `Write` instance, etc). Right now there is a blanket impl for `Fn() -> T where T: Write`. This works fine with functions like `io::stdout`. However, the _other_ common case for this trait is locking a shared writer. Therefore, it makes sense to see an implementation like this: ``` rust impl<'a, W: io::Write> MakeWriter for Mutex<W> where W: io::Write, { type Writer = MutexWriter<'a, W>; fn make_writer(&self) -> Self::Writer { MutexWriter(self.lock().unwrap()) } } pub struct MutexWriter<'a, W>(MutexGuard<'a, W>); impl<W: io::Write> io::Write for MutexWriter<'_, W> { // write to the shared writer in the `MutexGuard`... } ``` Unfortunately, it's impossible to write this. Since `MakeWriter` always takes an `&self` parameter and returns `Self::Writer`, the generic parameter is unbounded: ``` Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber) error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates --> tracing-subscriber/src/fmt/writer.rs:61:6 | 61 | impl<'a, W: io::Write> MakeWriter for Mutex<W> | ^^ unconstrained lifetime parameter error: aborting due to previous error ``` This essentially precludes any `MakeWriter` impl where the writer is borrowed from the type implementing `MakeWriter`. This is a significant blow to the usefulness of the trait. For example, it prevented the use of `MakeWriter` in `tracing-flame` as suggested in tokio-rs#631 (comment). ## Proposal This PR changes `MakeWriter` to be generic over a lifetime `'a`: ```rust pub trait MakeWriter<'a> { type Writer: io::Write; fn make_writer(&'a self) -> Self::Writer; } ``` The `self` parameter is now borrowed for the `&'a` lifetime, so it is okay to return a writer borrowed from `self`, such as in the `Mutex` case. I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`. Unfortunately, this is a breaking change and will need to wait until we release `tracing-subscriber` 0.3. Fixes tokio-rs#675. Signed-off-by: Eliza Weisman <[email protected]>

davidbarsky requested changes Mar 11, 2020

View reviewed changes

hawkw requested changes Mar 11, 2020

View reviewed changes

davidbarsky reviewed Mar 11, 2020

View reviewed changes

tracing-flame/Cargo.toml Outdated Show resolved Hide resolved

hawkw reviewed Mar 12, 2020

View reviewed changes

tracing-flame/src/error.rs Outdated Show resolved Hide resolved

tracing-flame/src/lib.rs Show resolved Hide resolved

tracing-flame/src/lib.rs Show resolved Hide resolved

tracing-flame/src/lib.rs Outdated Show resolved Hide resolved

hawkw reviewed Mar 12, 2020

View reviewed changes

tracing-flame/src/lib.rs Outdated Show resolved Hide resolved

tracing-flame/src/lib.rs Outdated Show resolved Hide resolved

yaahc commented Mar 12, 2020

View reviewed changes

yaahc and others added 10 commits March 13, 2020 15:29

Initial commit of tracing-flame

ece70e7

Apply suggestions from code review

7c734b0

Co-Authored-By: Eliza Weisman <[email protected]> Co-Authored-By: David Barsky <[email protected]>

Update tracing-flame/Cargo.toml

3f6a8c8

Co-Authored-By: David Barsky <[email protected]>

Fix for buffers and double panics

bc393e7

Add example of using inferno with flame_layer

c725d0d

cargo fmt

dff4c64

Add a tracing branded error reporter

5d11b17

Hide error enum kinds

2a44b26

fmt

22345e1

add module path to flame output

65f13bf

yaahc force-pushed the jane/flame branch from e379396 to 65f13bf Compare March 13, 2020 22:30

yaahc added 4 commits March 13, 2020 17:06

cargo clippy

958bb1c

Always acquire drop guard in with_file

6fbb3ae

remove FlameGuard from external api

9290f53

stylin

32fd5b0

davidbarsky mentioned this pull request Apr 2, 2020

ra_prof: use tracing? rust-lang/rust-analyzer#3794

Closed

Switch to thread local timestamp for flame samples

4601014

davidbarsky reviewed Apr 2, 2020

View reviewed changes

tracing-flame/Cargo.toml Outdated Show resolved Hide resolved

tracing-flame/tests/collapsed.rs Outdated Show resolved Hide resolved

tracing-flame/src/lib.rs Outdated Show resolved Hide resolved

yaahc added 4 commits April 2, 2020 11:19

use tempdir to cleanup files from tests

488a925

add support for arbitrary flushing via the guard

d45491e

Add docs for everythinggg

bc3bb68

Fix broken link

0060b3d

hawkw reviewed Apr 2, 2020

View reviewed changes

yaahc and others added 2 commits April 2, 2020 17:39

remove kind from public interface

e4da6ef

Apply suggestions from code review

490efc2

Co-Authored-By: Eliza Weisman <[email protected]>

hawkw reviewed Apr 8, 2020

View reviewed changes

yaahc and others added 4 commits April 8, 2020 17:30

Apply suggestions from code review

27687b8

Co-Authored-By: Eliza Weisman <[email protected]>

address comments

e57da20

fix the start time bug

bcea4a6

Add cargo metadata

d39fe9f

hawkw requested changes Apr 9, 2020

View reviewed changes

yaahc and others added 3 commits April 9, 2020 16:46

Apply suggestions from code review

3fab307

Co-Authored-By: Eliza Weisman <[email protected]>

tweet toml

b614ff4

fix the flame for concurrent programs

9a41fde

hawkw mentioned this pull request Apr 10, 2020

subscriber: MakeWriter probably needs to be generic over a lifetime #675

Closed

davidbarsky approved these changes Apr 21, 2020

View reviewed changes

hawkw approved these changes Apr 21, 2020

View reviewed changes

tracing-flame/src/lib.rs Outdated Show resolved Hide resolved

tracing-flame/tests/concurrent.rs Show resolved Hide resolved

Fix eliza comments

93b4c50

yaahc merged commit 280b664 into master Apr 21, 2020

hawkw mentioned this pull request Jul 6, 2020

subscriber: add lifetime parameter to MakeWriter #781

Merged

hawkw mentioned this pull request Oct 19, 2021

subscriber: add lifetime parameter to MakeWriter (#781) #1654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `tracing-flame` crate for generating flamegraphs/flamecharts #631

Add `tracing-flame` crate for generating flamegraphs/flamecharts #631

yaahc commented Mar 11, 2020

hawkw left a comment

yaahc left a comment

davidbarsky commented Apr 2, 2020

hawkw left a comment

hawkw left a comment

hawkw Apr 8, 2020

yaahc Apr 9, 2020

hawkw Apr 9, 2020

hawkw Apr 8, 2020

yaahc Apr 9, 2020

hawkw Apr 9, 2020

hawkw commented Apr 9, 2020

hawkw left a comment

hawkw Apr 9, 2020

davidbarsky Apr 9, 2020

davidbarsky left a comment

hawkw left a comment

		"Eliza Weisman <[email protected]>",
		"David Barsky <[email protected]>",

Add tracing-flame crate for generating flamegraphs/flamecharts #631

Add tracing-flame crate for generating flamegraphs/flamecharts #631

Conversation

yaahc commented Mar 11, 2020

hawkw left a comment

Choose a reason for hiding this comment

yaahc left a comment

Choose a reason for hiding this comment

davidbarsky commented Apr 2, 2020

hawkw left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Apr 9, 2020

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Add `tracing-flame` crate for generating flamegraphs/flamecharts #631

Add `tracing-flame` crate for generating flamegraphs/flamecharts #631