Make Tracing Configurability More Robust

[d-ulyanov]

Hi, trickster team!

We use Jaeger Agents to deliver trace data.
Currently, it's impossible to use agents because Collector initialization is hardcoded in init of Jaeger exporter:
https://github.com/Comcast/trickster/blob/master/internal/util/tracing/tracer.go#L33

Also, the tracing configuration seems quite inflexible at the moment, I think we need to reconsider tracer initialization totally.

[jranson]

@d-ulyanov thanks for the report. This is our first time working with Jaeger (we use a different tracing solution internally here @ Comcast) so I am sure there is lots of room for improvement. If you have any additional info or suggestions to make it flexible enough to work as desired for everyone, we'd love to hear them. We'll use this issue to track those improvements. cc: @guygrigsby

[guygrigsby]

@d-ulyanov Good feedback. Like James said, I haven't used much Jaeger. Let me see if I have the use case right. Rather than the Jaeger exporter, you would like to have Trickster report to the Jaeger agent running on a local host? Is it possible that the Trickster config could be set to export to the agent? Genuinely curious.

I'll start doing some research and see what I can find. Is there anything else about the way you are using Jaeger / OTEL that doesn't match up with Trickster?

[d-ulyanov]

https://github.com/open-telemetry/opentelemetry-go/blob/master/exporter/trace/jaeger/uploader.go#L25

Let's add some flexibility here, it would be nice to have additional factory for every provider, like

func SetTracer(tracerConfigs, tracerName) {

    // simplified code :)
    tracerConfig = tracerConfigs[tracerName]

    ...
    switch (tracer) {
    case Jaeger: 
       tracer = NewJaegerExporter(tracerConfig)
    case StackDriver: 
       tracer = NewStackDriverExporter(tracerConfig)
    // ... etc
    }

...
}

[jranson]

We are doing this already (see https://github.com/Comcast/trickster/blob/master/internal/util/tracing/tracer.go#L21). The StackDriver, I believe, was just made available for the golang libraries recently, and we'll get that support baked into a future release.

Are there any specific configurations or options that would improve the flexibility (e.g., some specific options that we have not implemented)?

[d-ulyanov]

Any tracer has its own specific if configuration, e.g. Jaeger has the remote sampling strategy, there is a possibility to send trace data to collectors or agents, etc.

Currently, it's not possible to provide tracer-specific configuration to this func:
https://github.com/Comcast/trickster/blob/master/internal/util/tracing/tracer.go#L21

[jranson]

I see, thanks! We do this with our caching layer already (e.g., options section for Redis vs bbolt, etc) and can do the same thing for Tracing providers. We will take a look @ the Jaeger options and make sure that they are configurable and exposed during Tracer registration, and work with you through this issue to make sure we have everything covered. We will slate this for the v1.1 release that should be about 3-4 weeks out.

[guygrigsby]

@jranson I'l start working in this.

[jmichalek132]

@jranson I'l start working in this.

any progress on this?

[jranson]

hey @jmichalek132 we are working this on the v1.1.x branch, and Guy has PRed a few improvements in #382, but we still have more work to do. We're going to be just a few days late on the 1.1. release (I anticipate next Monday) that will include these updates.

[jmichalek132]

hey @jmichalek132 we are working this on the v1.1.x branch, and Guy has PRed a few improvements in #382, but we still have more work to do. We're going to be just a few days late on the 1.1. release (I anticipate next Monday) that will include these updates.

Great thanks for update and the quick reply.

[jranson]

All, Trickster v1.1-beta1 is released, which includes the improvements to Tracing we've discussed, and some other great additions.

In addition to now supporting both Jaeger collector and agent (thanks Guy!), we added in support for custom service name, providing authentication credentials when shipping traces to the trace collector, custom Process tags included with every span, and also added support for Zipkin. In addition to these new options, the [tracing] configuration format is changed slightly (e.g., collector > collector_url), so be sure to check the tracing section of the example conf

One of the asks was that we support more of the Jaeger-specific features, such as Remote Sampler. The not-so-great news is that, since we are implementing Tracing with the opentelemetry-go packages, and we are early adopters, we are limited somewhat by otel's feature roadmap. Good news is that, since otel is gaining support and momentum, new features are coming out regularly. There is even an issue filed for Jaeger Remote Sampler support and work is in-progress (nice!). As soon as that support comes in a future release of otel, we will also implement the passthrough in Trickster. I'll keep this issue open until Remote Sampler support is fully completed.

Also, with this beta release (and going forward) we provide a trickster-demo Docker Compose, which includes working, try-able tracing configuration examples out-of-the-box. Let us know if you have any feedback on that, because we'd love for it to be as useful and robust as possible.

One area where we want to improve is the quality of the span. For example, appropriate span tagging, like cache name, origin name, etc., whenever useful. If you have any feedback on how we can improve those aspects, we'd love to hear that as well. Thanks again!