V2

[inanna-malick]

major changes, refactoring to use new tracing-subscriber tek

[hawkw]

i haven't reviewed everything yet, but here are a few notes on what i've looked at so far.

[hawkw]

seems like you could just use the existing SpanRef::parents iterator for this:

let iter = ctx.span(id).iter().flat_map(|span| span.parents());

or something

[hawkw]

this whole module feels honeycomb-specific...I believe other distributed tracing systems have their own notions of trace and span IDs that might have different representations, and the format these are parsed from feels honeycomb-specific. what do you think about replacing the implementation in the dist-tracing crate with an interface and moving this implementation to the honeycomb-tracing crate?

I think the Telemetry trait could have associated types defining IDs, something like

pub trait Telemetry {
    type TraceId: Clone + Hash + Eq + FromStr + fmt::Display;
    type SpanId: Clone + Hash + Eq + FromStr + fmt::Display;
    // ...
}

or something?

[inanna-malick]

This is almost certainly the right thing to do, but it'll result in a small usability hit - I've been trying to keep the out-of-band functions (TraceCtx::new_root().record_on_current_span(), TraceCtx::current_trace_ctx()) free of type parameters.

I suppose it would be fine to export specialized wrapper functions from the honeycomb-tracing crate, and to expect any other such crates to do so as well, and the small bit of extra boilerplate would be worth it for the added flexibility.

[inanna-malick]

update: implemented this

[hawkw]

seems like this could be

let iter = span.parents();

[inanna-malick]

That code is a bit messy, but the iter I construct w/ unfold also includes the current span. I'm not sure how to modify this to use span.parents, my intuition is to write something like

std::iter::once(span).chain(span.parents());

but that results in a move error:

error[E0382]: borrow of moved value: `span`
   --> dist-tracing/src/telemetry_layer.rs:213:49
    |
211 |         let span = ctx.span(&id).expect("span data not found during on_close");
    |             ---- move occurs because `span` has type `tracing_subscriber::registry::SpanRef<'_, S>`, which does not implement the `Copy` trait
212 | 
213 |         let iter2 = std::iter::once(span).chain(span.parents());
    |                                     ----        ^^^^ value borrowed here after move
    |                                     |
    |                                     value moved here

[davidbarsky]

Nit: I'd suggest writing this instead as:

let subscriber = registry::Registry::default() // provide underlying span data store
    .with(LevelFilter::INFO) // filter out low-level debug tracing (eg tokio executor)
    .with(tracing_subscriber::fmt::Layer::builder().finish()) // log to stdout
    .with(telemetry_layer); // publish to tracing

It makes the hierarchy somewhat cleaner and closer to other frameworks.