Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Decouple Tracer and Carrier Concerns #184

Closed
abraithwaite opened this issue May 9, 2018 · 9 comments
Closed

Decouple Tracer and Carrier Concerns #184

abraithwaite opened this issue May 9, 2018 · 9 comments

Comments

@abraithwaite
Copy link

abraithwaite commented May 9, 2018

Problem

Currently, in order to serialize a opentracing.SpanContext onto a map, you must first initialize a tracer and use Inject.

This is problematic because often times, you don't want or need a tracer implementation to be able to serialize a particular context.

Additionally, it leads coders of tacer implementations to duplicate nearly the exact same code, except totally incompatible with one another (because of inconsistent prefixing):

// Inject implements the Injector interface
func (t *TextMapPropagator) Inject(spanContext MockSpanContext, carrier interface{}) error {
writer, ok := carrier.(opentracing.TextMapWriter)
if !ok {
return opentracing.ErrInvalidCarrier
}
// Ids:
writer.Set(mockTextMapIdsPrefix+"traceid", strconv.Itoa(spanContext.TraceID))
writer.Set(mockTextMapIdsPrefix+"spanid", strconv.Itoa(spanContext.SpanID))
writer.Set(mockTextMapIdsPrefix+"sampled", fmt.Sprint(spanContext.Sampled))
// Baggage:
for baggageKey, baggageVal := range spanContext.Baggage {
safeVal := baggageVal
if t.HTTPHeaders {
safeVal = url.QueryEscape(baggageVal)
}
writer.Set(mockTextMapBaggagePrefix+baggageKey, safeVal)
}
return nil
}

https://github.com/lightstep/lightstep-tracer-go/blob/ad2fc76119df92fc9d6f9bc14dc15fe481074006/propagation_text.go#L24-L44
https://github.com/jaegertracing/jaeger-client-go/blob/b7bacc465f385658dd3586f58810226776d2a4bf/propagation.go#L103-L122

This is somewhat related to #127.

Use Case

We want to be able to serialize the SpanContext without initializing a tracer. Useful for testing and cross-compatibility between tracing clients.

In one particular case, we're trying to embed a span context onto a JSON document using a text map and we are trying to avoid introducing another dependency (the tracer). In this particular program, it would be nice to not have to depend on an external dependency such as Jaeger or Lightstep.

Proposal

Conceptually, implementing this is easy. Just separate the existing carrier logic from the tracer implementation. Realistically it would be a challenge to do this in a backwards compatible way.

I think the ideal case given the state of things would be to introduce new APIs which separate the two concerns.

Something like (not necessarily equal to):

sCtx, err := opentracing.Serialize(spanContext)
opentracing.InjectSerialized(sCtx, req.Header)

wireCtx, err := opentracing.ExtractSerialized(req.Header)
spanContext, err := opentracing.Deserialize(wireCtx)
@yurishkuro
Copy link
Member

I am not clear what problem this is trying to address.

We want to be able to serialize the SpanContext without initializing a tracer.

Why?

Useful for testing and cross-compatibility between tracing clients.

First, this is assuming that compatibility between tracing clients from different vendors is a requirement. It's not a requirement of OpenTracing - they may very well be compatible (esp. with W3C Trace Context work), but OpenTracing is not dictating that. Second, if two implementations (e.g. Jaeger and Zipkin tracers) do desire to be compatible on the wire, there is no need to change OpenTracing APIs for that, we can write a program that instantiates these two tracers and uses standard Inject/Extract methods to test that they each understand the other tracer's SpanContext wire representation.

@abraithwaite
Copy link
Author

Why?

Mentioned following that sentence, but to repeat:

  • The separation makes testing code which implements or depends on tracing easier.
  • It allows us to focus on the medium of communication instead of the actual implementation of the tracers.

I understand that compatibility is not a requirement of OpenTracing, but it makes the library a lot easier and enjoyable to use if they are compatible.

Second, if two implementations (e.g. Jaeger and Zipkin tracers) do desire to be compatible on the wire, there is no need to change OpenTracing APIs for that

Apologies, I may not have described this well enough. My concern is less about them being compatible and more about the core separation of concerns between the two components of the software. One is for serialization onto some medium (carrier). The other is to transport spans to a central location (tracer). I don't see why these necessarily must be tightly coupled as they are in the current architecture.

For more context, we're trying to embed a span context onto a JSON document using a text map and we are trying to avoid introducing another dependency (the tracer). In this particular program, it would be nice to not have to depend on an external dependency such as Jaeger or Lightstep.

We could use the mocktracer or implement our own tracer, but we will be just be copying the code which has already been copied N times and introducing more surface area for bugs where there needn't be any. If Opentracing were to separate the two concerns (trace systems and carriers) this would be totally unnecessary.

Lemme know if I can clarify anything more.

@abraithwaite
Copy link
Author

Updated the ticket with a particular example.

@yurishkuro
Copy link
Member

Seems what you want is a general distributed context propagation framework, without the tracing (which can be built on top of such framework), such as https://github.com/tracingplane/tracingplane-java/

@yurishkuro
Copy link
Member

Ultimately you are proposing adding a requirement that all tracing systems use the same wire format for the span context. Like I said, this has never been a requirement of OT project, and I don't see how your proposed API change can happen without introducing such requirement.

@abraithwaite
Copy link
Author

abraithwaite commented May 9, 2018

Ultimately you are proposing adding a requirement that all tracing systems use the same wire format for the span context. Like I said, this has never been a requirement of OT project, and I don't see how your proposed API change can happen without introducing such requirement.

It seems that it wouldn't be possible to do this in any other way, yes. If Opentracing doesn't provide a true drop-in solution for switching between frameworks by taking a stand on this front, where is the value in using it over a small wrapper around the provider implementations themselves?

Edit: Thinking more on this, I don't think it would be necessary for tracers to share wire format. See below.

@yurishkuro
Copy link
Member

may I recommend this talk? https://www.youtube.com/watch?v=-zLPPYYH_F8

tl;dr; - opentracing enables vendor-independent instrumentation of open source and internal frameworks.

The instrumentation has no concerns with wire format, it only talks to an API implemented by a tracer. To test the instrumentation you can use a mock tracer. Compare this with a logging API. When you say logger.Info(...), do you know/care if the output will be written in plain text, JSON, kafka, whatever? That line is the instrumentation. If you run an application with that line, then you care, by providing an implementation of the logger API.

@abraithwaite
Copy link
Author

Thanks for sharing the talk, it was very informative.

I think this comment highlights my frustration:

Compare this with a logging API. When you say logger.Info(...), do you know/care if the output will be written in plain text, JSON, kafka, whatever?

In order to do anything useful with opentracing, we must have a tracer provider. So by that means, we must care about what the output will be before we can have any useful output whatsoever. By only defining interfaces and not defining any concrete data types it puts the onus on the developers to know all this ahead of time (in the program) in order to work with this library.

To continue with the logging example: I'm sure if we dig into logging libraries they'll have concrete types generally resembling what a log line is before handing it off to adapters to be sent wherever.

Does that make sense? Maybe I'm crazy and I'm missing the point somewhere though. 😞

@yurishkuro
Copy link
Member

Well, we also have a default implementation - it does nothing. There is a subtle difference with logging - to implement a logger, all you need is a println(). To implement a tracer, especially the one that talks to a remote tracing backend, it's a little bit more work. So "do nothing" is not a bad default for a tracer, since you haven't told us which tracing backend you want to use.

We could go a bit further and say that default behavior you want is to propagate distributed context, but not record any traces. You can use Jaeger with sampling off for that. If you don't like Jaeger - implement your own propagation-only thing. The point is, it's no longer a default implementation because all existing tracing systems define their own propagation format, and it's not the place of OpenTracing API to force them to some common format. If someone does manage to do that (the W3C project I mentioned), then we will add a 2nd "default" implementation to OpenTracing that uses W3C format for propagation. Whether it will be the primary or secondary default - tbd, since there are perf implications of using anything that is not a noop tracer. But even in this case, this propagation format is not going to be required by the OpenTracing API, because if it were it would break all existing tracing installations that are already using custom formats.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants