Optional serde support for SpanContext

[shaun-cox]

Fixes #1074

Changes

Follow Rust API Guidelines in regards to SpanContext data structure implementing Serde's Serialize and Deserialize traits, optionally based on "serde" feature flag.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: shaun-cox / name: Shaun (e03e474, afed817, ac1c068)

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.1 ⚠️

Comparison is base (0a893ce) 50.9% compared to head (ddcc8d2) 50.8%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1075     +/-   ##
=======================================
- Coverage   50.9%   50.8%   -0.1%     
=======================================
  Files        165     165             
  Lines      19689   19697      +8     
=======================================
  Hits       10025   10025             
- Misses      9664    9672      +8     

Impacted Files	Coverage Δ
opentelemetry-api/src/trace/span_context.rs	89.4% <0.0%> (-2.2%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hdost]

Makes sense and having behind a feature flag is good 👍🏼

[jtescher]

I think this seems a little against the spirit of the propagator spec as this would suggest a simple way to bypass that API, but I don't feel strongly enough to block if others find this useful. @TommyCpp thoughts?

[djc]

I think this seems a little against the spirit of the propagator spec as this would suggest a simple way to bypass that API, but I don't feel strongly enough to block if others find this useful. @TommyCpp thoughts?

Since we've had a few people speak up in favor of this in #576, it feels like we should accept it.

@jtescher what are the risks/issues with bypassing the propagator?

[wperron]

I agree this seems odd to implement on SpanContext, if I understand correctly the discussion in #576, serde support was mainly as a way to support custom exporters. If instead we implement serde on the sdk's SpanData struct, it could support that use case without creating ambiguity for propagation.

[djc]

@shaun-cox what do you think about SpanData vs SpanContext? What's your use cases for specifically wanting to (de)serialize SpanContext?

[shaun-cox]

@djc, @wperron, @jtescher thanks for the feedback and questions and patience as I attempt to learn and contribute here.

My use case is in in the context of a library that I'm wanting to instrument. This library communicates with other instances of itself over the network using a binary protocol. That binary protocol is already relying on serde and is performance sensitive. My aim is simply to enable distributed tracing across this library's network boundary.

My first thought was that I should limit myself to opentelemetry-api types, and not have my library take any dependency on opentelemetry-sdk, as I'm not building a generic exporter or anything, just wanting to stick to the "instrumenting" use case, but carry the necessary minimum context across my boundary.

I looked at SpanData, but it's not readily apparent to me how to leverage it for my use case. I'd like to avoid TextMapPropagator (for now) as it seems aimed at HTTP/W3C type stuff, which feels a lot more heavy-weight than what I need. IOW, I'd like to avoid allocating a temporary HashMap<String, String> via TextMapPropagator then to hand that to serde to serialize into my binary protocol.

I thought I was on the right track with this PR approach, after noticing that TraceContextExt.with_remote_span_context was the way to construct a new (correlated) Context at the receive side of my binary protocol after providing the deserialized SpanContext. But if that is not the right approach, I'd love to be educated more...

I also noticed that opentelemetry-api::propagation module has some comments about BinaryFormat, but no implementation yet, so wasn't sure if there might be contributions needed in this area to implement the spec more? I might be up for contributing there, if needed, and if it ties in with my use case better.

To be clear, I want to help push opentelemetry-rust along as I like what I see so far, and I don't just want to add a patch that suits my use case if it's not really "in line" with the general open telemetry vision/spec, which I am still learning.

Thanks!

[jtescher]

I also noticed that opentelemetry-api::propagation module has some comments about BinaryFormat, but no implementation yet, so wasn't sure if there might be contributions needed in this area to implement the spec more? I might be up for contributing there, if needed, and if it ties in with my use case better.

Is the BinaryFormat in contrib what you're looking for?

[shaun-cox]

I've also since understood that binary format propagators have been removed from the spec, and there is an issue #437 to address adding them back at some point.

I think I understand the need for "open" binary protocols to propagate context, but I hope the community sees the interim need for context to be opaquely propagated inside "closed" binary protocols in the meantime, which is how I view my use case.

Is the BinaryFormat in contrib what you're looking for?

Oh, thanks! I'll check it out...

[wperron]

I think I understand the need for "open" binary protocols to propagate context, but I hope the community sees the interim need for context to be opaquely propagated inside "closed" binary protocols in the meantime, which is how I view my use case.

My issue here is that this is possible to do using the Propagator traits already. It's not clear to me how Serialize/Deserialize are supposed to interact with Injector/Extractor. I can see them being used in conjunction with one another, but for spec compliance reasons, Injector/Extractor must be the canonical way to propagate context across service boundary.

I'm not necessarily opposed to this change, I would just like to see a better use-case defined; Why do the existing traits not work for you? What are the possible drawbacks or footguns that come with this change?

[djc]

I can see them being used in conjunction with one another, but for spec compliance reasons, Injector/Extractor must be the canonical way to propagate context across service boundary.

Do we know why the spec mandates this? Leaving out useful features for spec compliance seems like a bad pattern. (And having a canonical way to do something doesn't maybe quite mandate that it's the only way?)

[wperron]

It's also worth pointing out that Binary Propagators are coming at some point to the specification, which will solve the inject to Map -> serialize to binary two-step awkwardness.

Maybe the best course of action would be an experimental BinaryPropagator in the contrib package?

[wperron]

I can see them being used in conjunction with one another, but for spec compliance reasons, Injector/Extractor must be the canonical way to propagate context across service boundary.

Do we know why the spec mandates this? Leaving out useful features for spec compliance seems like a bad pattern. (And having a canonical way to do something doesn't maybe quite mandate that it's the only way?)

I'm really not sure. I'm just an interested party here, might be worth asking in the otel-specification channel in the CNCF Slack.

Again though I'm not opposed to having serde in the mix, I'm sure there's tons of values in being able to serialize span data to JSON say, or whatever other format, but I do think that if we're talking about Propagation specifically, we should stick to the standard.

Or, put differently, the api package should be as close to the standard as possible, and the sdk package can take some liberties. Which is why I was suggesting implementing serde on SpanData, as that's part of the sdk package, and imo the use-case for serde in the context of an Exporter is much stronger.

[shaun-cox]

@wperron wrote:

Maybe the best course of action would be an experimental BinaryPropagator in the contrib package?

Yeah, jtescher pointed out above that it's there at BinaryFormat. I'll probably use that in the meantime, though it doesn't integrate with any Injector/Extractor abstractions.

All it does is serialize the trace_id, span_id, and trace_flags constituents of SpanContext, and does it using the to_bytes() methods on each of those. (One could even argue/wonder why SpanContext doesn't just have a to_bytes() and from_bytes() methods as its constituents do, which is arguably as idiomatic for Rust as using serde for serialization.

I am also interpreting the following, from CONTRIBUTING.md

It is preferable to have contributions follow the idioms of the language rather than conform to specific API names or argument patterns in the spec.

[TommyCpp]

I think this seems a little against the spirit of the propagator spec as this would suggest a simple way to bypass that API, but I don't feel strongly enough to block if others find this useful. @TommyCpp thoughts?

I think it's fine to introduce the serde trait behind a feature flag. It will be helpful for library developers to implement custom propagator and prevent end users to using it in accident

[shaun-cox]

I also updated the PR to not actually serialize the is_remote field, and on deserialization, to set it to true, which honors the intention of the spec, I believe.

[jtescher]

Something worth noting is that span context serialization is standardized through the propagation apis so that environments with multiple language implementations are more likely to properly propagate the context correctly. In that sense users should be discouraged from implementing their own serialization formats unless there is a compelling reason to do so (e.g. noticing that span context implements Serialize and propagating as json via serde_json would not generally be a recommended approach).

As we get closer to 1.0 for the trace signal we should be clear about what API guarantees we want to make, and how lower context readers will interpret and use the interfaces we expose.

[hdost]

Something worth noting is that span context serialization is standardized through the propagation apis so that environments with multiple language implementations are more likely to properly propagate the context correctly. In that sense users should be discouraged from implementing their own serialization formats unless there is a compelling reason to do so (e.g. noticing that span context implements Serialize and propagating as json via serde_json would not generally be a recommended approach).

As we get closer to 1.0 for the trace signal we should be clear about what API guarantees we want to make, and how lower context readers will interpret and use the interfaces we expose.

That is a good point it would certainly hinder the library as pushing internals out as a public interface could cause pain in the future.

[shaun-cox]

We chatted briefly about this in the Rust SIG meeting today. I'm happy to close this and go think more on how to use binary propagators in my own protocol. We decided to leave the PR open in the meantime for more discussion/context.

[shaun-cox]

While binary propagator contrib stuff wasn't applicable to my use case, I was able to work around with my own struct which is convertible from SpanContext, so closing this.