opentelemetry: Add extension to link spans

[LehMaxence]

Repeat of #1480 to merge on master.

Motivation

Discussed in #1121, the opentelemetry specifications allow adding a link to a propagated span and/or a closed span. However, the implemented on_follows_from of the OpenTelemetryLayer does not allow this.

Solution

The solution follows the same model as the set_parent Span extension function.
A add_link function that takes the linked span context was added to the OpenTelemetrySpanExt.

[hawkw]

looks good to me! commented on a couple of minor things, but no blockers.

let's see if @jtescher has anything to add?

[hawkw]

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.

[jtescher]

add_link_with_attributes sounds good to me 👍

[hawkw]

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?

[LehMaxence]

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

[hawkw]

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?

[LehMaxence]

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)

[jtescher]

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.

[hawkw]

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]);
	}
	});
	}