+
Skip to content

Conversation

inanna-malick
Copy link
Owner

major changes, refactoring to use new tracing-subscriber tek

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.

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

Comment on lines 182 to 191
let iter = itertools::unfold(Some(parent_id.clone()), |st| match st {
Some(target_id) => {
let res = ctx
.span(target_id)
.expect("span data not found during eval_ctx");
*st = res.parent().map(|x| x.id());
Some(res)
}
None => None,
});
Copy link

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,255 @@
use chrono::{DateTime, Utc};
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

update: implemented this

Comment on lines 215 to 225
// TODO: dedup
let iter = itertools::unfold(Some(id.clone()), |st| match st {
Some(target_id) => {
let res = ctx
.span(target_id)
.expect("span data not found during eval_ctx");
*st = res.parent().map(|x| x.id());
Some(res)
}
None => None,
});
Copy link

Choose a reason for hiding this comment

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

seems like this could be

let iter = span.parents();

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

@inanna-malick inanna-malick merged commit 018cc55 into master Apr 7, 2020
Fishrock123 added a commit to Fishrock123/tracing-honeycomb that referenced this pull request Apr 15, 2021
- Relaxed dependency constraints.
    - Was included in 0.2.1-eaze.2
- Fixed a panic in case of multiple current callers. See [inanna-malick#3](eaze/tracing-honeycomb#3).
    - Was included in 0.2.1-eaze.3
- Added `parking_lot` feature.
    - Was included in 0.2.1-eaze.2
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.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载