Conversation
hawkw
left a comment
There was a problem hiding this comment.
this is awesome --- when this PR is released, it would be great to add some stuff to the tokio-console documentation about using it with timely-dataflow!
i left a few suggestions, but i don't know much about how timely-dataflow works, so I may be wrong about some things!
| // timely.console.span is a marker target allowing us to | ||
| // turn these spans on and off specifically | ||
| // TODO(guswynn): check with tracing folks if there is a better | ||
| // way to do this | ||
| tracing::trace_span!( | ||
| target: "timely.console.span", |
There was a problem hiding this comment.
in general, tracing targets are Rust module paths when they're not explicitly overrididen; so :: is typically used as a separator rather than . --- the use of . is not forbidden by tracing, but it's not the general convention.
i don't really understand the role of the timely::console::span target --- i would probably just make this
| // timely.console.span is a marker target allowing us to | |
| // turn these spans on and off specifically | |
| // TODO(guswynn): check with tracing folks if there is a better | |
| // way to do this | |
| tracing::trace_span!( | |
| target: "timely.console.span", | |
| tracing::trace_span!( | |
| target: "timely::dataflow::operator", |
or something if you don't want to use the full module path (the user probably doesn't care about ::generic::builder_raw here?). you'll still be able to filter that target out without having a weird target for it.
There was a problem hiding this comment.
I want to have a set target so that if this code ever moves, set filters in our downstream binary don't break, but good point, timely::dataflow::operator is probably better!
| kind = %"timely-operator", | ||
| "fn" = %std::any::type_name::<L>(), | ||
| task.name = self.shape.name.as_str(), |
There was a problem hiding this comment.
i don't know anything about timely dataflow, so...is shape_name something provided by the user that can describe a particular task in the user code, or is it provided by the library? if it's internal to the library and describes a general class of dataflow operator, i would probably use that as the kind field rather than the task.name field --- in Tokio's instrumentation, the kind field will have values like "task", for tasks spawned via tokio::spawn, "local", for tasks spawned via spawn_local, "blocking" for blocking-pool tasks, and "block_on" for tasks created by tokio::runtime::block_on.
if shape_name is something internal to timely dataflow, i would probably use that for the kind field --- the target of the span will still indicate that it's coming from timely. if it's provided by the user, though, this seems correct.
There was a problem hiding this comment.
Yeah, it is something that people set if using the raw builder api, or wrappers around it like unary_frontier
For other combinators, like flat_map, consolidate, etc., its set to a uppercase version of the function name.
I think it would require more thorough changes throughout timely to distinguish between these, but using kind is a great point for a followup
things like
timely_stream.flat_map(|...| logic)
would end up with
kind = "FlatMap"
name = "FlatMap"
location = hopefully_the_top_level_callsite
target = timely::whatever
(and possible named version of the same functions in the future
and things like
timely_stream.unary_frontier(p_contract, "my_special_name", || logic)
kind = "UnaryFrontier"
name = "my_special_name"
location = hopefully_the_top_level_callsite
target = timely::whatever
I need to go through and add #[track_caller] to all these combinators anyways, so it might be nice to a do extra kind/name refactoring at the same time
…and optionally instrument it for the tokio-console
As discussed with @antiguru, this adds and optional feature that instruments timely operators, as
Outstanding things to do (not necessarily in this pr)
#[track_caller]on all the operator-combinators in timely (and also in dd, if that makes sense with collections). This will make the "spawn site" field in the console more accurate, right now it just points to this function. Local testing shows me that it works on just the trait method declarations!_namedversions of combinators?