-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Make frame spans appear on a separate trace line #143955
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
base: master
Are you sure you want to change the base?
Conversation
r? @RalfJung |
This was done by making the tracing_chrome tracing layer check if "tracing_separate_line" was in the arguments of a span, and act accordingly.
ef066d8
to
d9ab992
Compare
fn get_root_id(&self, span: SpanRef<S>) -> Option<u64> { | ||
match self.trace_style { | ||
TraceStyle::Threaded => { | ||
if span.fields().field("tracing_separate_line").is_some() { | ||
// put spans with argument "tracing_separate_line" on a separate trace line | ||
// by setting their "id", see | ||
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.jh64i9l3vwa1 | ||
Some(span.id().into_u64()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe tracing_root
or so would be a better name?
tracing_separate_line
was just the first thing that came to my mind without understanding anything about what actually happens or what the tracing terminology is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You initially suggested tracing_separate_root
and I changed it to tracing_separate_line
; I settled on tracing_separate_line
because that's what can be observed in the UI, i.e. those spans appear on a separate line. I tried to think of better names but I believe there is no actual technical term: the documentation of the trace format (also cited there) just calls the field "id" which per se does not mean much, and the tracing_chrome
crate calls it "root_id" which again does not mean much and is probably arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that it's a "line" is just a choice of that one UI though, right? Seems odd to hard-code it in the naming.
You can emit async events from different processes and different threads. Each async event has an additional required parameter id. We consider the events with the same category and id as events from the same event tree.
So maybe tracing_separate_thread
?
(That makes me wonder whether we can use this to resolve rust-lang/miri#2266, but that's a separate discussion.)
This PR changes tracing_chrome's
tracing::Layer
so that if a span has the "tracing_separate_line" field as one of the span arguments, that span is put on a separate trace line. See rust-lang/miri#4451 for an earlier attempt and for screenshots explaining better what I mean by "separate trace line".This PR also makes the "frame" span use this feature (so it appears on a separate trace line, see rust-lang/miri#4451 for motivation), but passes
tracing::field::Empty
as the span parameter value so it is ignored by other tracing layers (e.g. the logger):Also see the following discussion I had with @RalfJung
These are the static metadata items we can control about a span. We can't add more metadata outside of them. The most relevant are:
name
(for the frame span it's currently "frame")target
which acts as the category (for the frame span it's currently "rustc_const_eval::interpret::stack" by default)fields
which contains a list of the names of each of the arguments passed to thespan!
macro (for the frame span it's currently ["message"], where "message" is the default identifier for data passed in theformat!
syntax)When the tracing code is called at runtime, the dynamic values of the arguments are collected into a
ValueSet
. Each argument value stored there corresponds with one of the static names stored infields
(see above).We have already determined that filtering out spans by
name
is not a good idea, and I would say the same goes fortarget
. Both thename
and thetarget
fields are printed to stderr whenMIRI_LOG=
is enabled, so changing them to contain an identifier (e.g. "frame:tracing_separate_root" instead of "frame" as the name) would uselessly clutter the text logs (unless we add one more filter there, but then it gets even more complicated).So that leaves us with
fields
and their runtime values. Now, my initial thought (inspired by this comment) was to use a field with the static name "tracing_separate_root" and with a dynamic boolean value of "true". Intracing_chrome.rs
we can easily check if this field is true and act accordingly. This would work but then again this field would also be picked up by the logger whenMIRI_LOG=
is enabled, and would uselessly clutter the text logs.To avoid cluttering the text logs, we can instead set "tracing_separate_root" to the dynamic value of
tracing::field::Empty
. Citing from here, "when a field’s value isEmpty
, it will not be recorded". "not being recorded" means that the field and its value won't be printed to stderr text logs, nor will it be printed by any other tracing layers that might be attached in the future. Intracing_chrome.rs
we would still be able to check if "tracing_separate_root" is in the list of staticfields
, and act accordingly. So I believe this solution would effectively allow us to attach metadata to a span in a way that does not clutter logs and still allows being read intracing_chrome.rs
.If we ever wanted to pass arbitrary metadata (i.e. not just a present/not present flag), it would be possible with a custom
Empty
that also holds data and implementValue
without doing anything (likeEmpty
does).