Skip to content

add tracing feature#452

Open
guswynn wants to merge 1 commit intoTimelyDataflow:masterfrom
guswynn:tracing
Open

add tracing feature#452
guswynn wants to merge 1 commit intoTimelyDataflow:masterfrom
guswynn:tracing

Conversation

@guswynn
Copy link

@guswynn guswynn commented Feb 25, 2022

…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!
    • (more accurate naming; unfortunately, combinators have the same default name always, so its unclear WHY a certain combinator exists. we can either capture this structure differently, or we can add like _named versions of combinators?
  • figure out what other information we want to put in this span. Right now the console does not show all fields, but in the future it will! This might help us track the stack of combinators, as names are somewhat confusing right now
  • Figure out if we want to/can model activations between operators as "resources" in the console.
  • ??? perf testing ???

Copy link

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines +168 to +173
// 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",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
// 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines +175 to +177
kind = %"timely-operator",
"fn" = %std::any::type_name::<L>(),
task.name = self.shape.name.as_str(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@guswynn guswynn Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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