Skip to content
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.

Sign up for GitHub

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

Jump to bottom

opentelemetry: Add extension to link spans #1516

Merged
merged 4 commits into from
Aug 23, 2021
Merged

opentelemetry: Add extension to link spans #1516

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab828d5
opentelemetry: Add extension to link spans
LehMaxence Jul 28, 2021
e8d3cce
Improve syntax
LehMaxence Aug 18, 2021
1ff0de6
Merge branch 'master' into opentelemetry-add-link
hawkw Aug 19, 2021
aafb661
Rework `add_link`
LehMaxence Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 55 additions & 1 deletion tracing-opentelemetry/src/span_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::subscriber::WithContext;
use opentelemetry::Context;
use opentelemetry::{trace::TraceContextExt, Context};

/// Utility functions to allow tracing [`Span`]s to accept and return
/// [OpenTelemetry] [`Context`]s.
Expand Down Expand Up @@ -42,6 +42,40 @@ pub trait OpenTelemetrySpanExt {
/// ```
fn set_parent(&self, cx: Context);

/// Associates `self` with a given OpenTelemetry trace, using the provided
/// followed span [`Context`].
///
/// [`Context`]: opentelemetry::Context
///
/// # Examples
///
/// ```rust
/// use opentelemetry::{propagation::TextMapPropagator, trace::TraceContextExt};
/// use opentelemetry::sdk::propagation::TraceContextPropagator;
/// use tracing_opentelemetry::OpenTelemetrySpanExt;
/// use std::collections::HashMap;
/// use tracing::Span;
///
/// // Example carrier, could be a framework header map that impls otel's `Extract`.
/// let mut carrier = HashMap::new();
///
/// // Propagator can be swapped with b3 propagator, jaeger propagator, etc.
/// let propagator = TraceContextPropagator::new();
///
/// // Extract otel parent context via the chosen propagator
/// let parent_context = propagator.extract(&carrier);
///
/// // Generate a tracing span as usual
/// let app_root = tracing::span!(tracing::Level::INFO, "app_start");
///
/// // Assign parent trace from external context
/// app_root.add_link(parent_context.clone());
///
/// // Or if the current span has been created elsewhere:
/// Span::current().add_link(parent_context);
/// ```
fn add_link(&self, cx: Context);
Copy link
Member

Choose a reason for hiding this comment

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

oh, one other thought. I notice that in the examples, the Context parameter always has to be cloned...but in the add_link method, we borrow the span from the context, and then clone the SpanContext from the span...so, afaict, we could just borrow the Context parameter rather than forcing the user to clone it when calling this method. that might be more efficient, as well?

but, on the other hand, existing methods on this trait (e.g. set_parent) take the Context by value, so maybe it's preferable to be consistent? IDK.

@jtescher what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to be consistent with set_parent, as you notice, but that may not be necessary indeed.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i think there's kind of a tradeoff between consistency and ergonomics (and possibly performance?) here...i'd probably go with taking it by reference, but it's @jtescher's call.

alternatively, should this method just take the SpanContext? that way, the user can get it from somewhere other than a Context...i'm not sure if there's a use-case for that, or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method could just take SpanContext since links don't care about the whole Context.
However propagators return Context so maybe we need both fn add_link(&self, cx: SpanContext) and fn add_link(&self, cx: &Context) .

fn add_link(&self, cx: &Context) would just get the SpanContext of the Context and pass it to fn add_link(&self, cx: SpanContext)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the add_link spec suggests a SpanContext directly, while the span creation spec explicitly requires a full Context to set a parent so the parameter discrepancy seems intentional.


/// Extracts an OpenTelemetry [`Context`] from `self`.
///
/// [`Context`]: opentelemetry::Context
Expand Down Expand Up @@ -86,6 +120,26 @@ impl OpenTelemetrySpanExt for tracing::Span {
});
}

fn add_link(&self, cx: Context) {
let mut cx = Some(cx);
self.with_collector(move |(id, collector)| {
if let Some(get_context) = collector.downcast_ref::<WithContext>() {
get_context.with_context(collector, id, move |builder, _tracer| {
if let Some(cx) = cx.take() {
let follows_context = cx.span().span_context().clone();
let follows_link =
opentelemetry::trace::Link::new(follows_context, Vec::new());
Copy link
Member

Choose a reason for hiding this comment

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

I notice this will always create a link with no key-value attributes. Do you think it makes sense to include a separate function like add_link_with_attributes(&self, cx: Context, attrs: Vec<KeyValue>), or something?

We can always add this in a separate PR, of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

add_link_with_attributes sounds good to me 👍

if let Some(ref mut links) = builder.links {
links.push(follows_link);
} else {
builder.links = Some(vec![follows_link]);
LehMaxence marked this conversation as resolved.
Show resolved Hide resolved
}
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

i think that if we borrowed the Context parameter, as in my above suggestion, the implementation would be a bit simpler:

Suggested change
fn add_link(&self, cx: Context) {
let mut cx = Some(cx);
self.with_collector(move |(id, collector)| {
if let Some(get_context) = collector.downcast_ref::<WithContext>() {
get_context.with_context(collector, id, move |builder, _tracer| {
if let Some(cx) = cx.take() {
let follows_context = cx.span().span_context().clone();
let follows_link =
opentelemetry::trace::Link::new(follows_context, Vec::new());
if let Some(ref mut links) = builder.links {
links.push(follows_link);
} else {
builder.links = Some(vec![follows_link]);
}
}
});
}
fn add_link(&self, cx: &Context) {
self.with_collector(|(id, collector)| {
if let Some(get_context) = collector.downcast_ref::<WithContext>() {
get_context.with_context(collector, id, |builder, _tracer| {
let follows_context = cx.span().span_context().clone();
let follows_link =
opentelemetry::trace::Link::new(follows_context, Vec::new());
if let Some(ref mut links) = builder.links {
links.push(follows_link);
} else {
builder.links = Some(vec![follows_link]);
}
});
}

});
}

fn context(&self) -> Context {
let mut cx = None;
self.with_collector(|(id, collector)| {
Expand Down