Add http and kafka transports to zipkin-tracer

[codefromthecrypt]

Many people aren't running scribe and don't want to run scribe. Zipkin used to only work with scribe, but it now works with http and kafka.

I've pasted a copy of a patched http raw zipkin tracer below.. Is something like this possible to add to Finagle directly?

  /** Logs spans via POST /api/v1/spans. */
  override def logSpans(spans: Seq[Span]): Future[Unit] = {
    // serialize all spans as a thrift list
    val serializedSpans = try {
      val transport = new TMemoryBuffer(0)
      val oproto = new TBinaryProtocol(transport)
      oproto.writeListBegin(new TList(TType.STRUCT, spans.size))
      spans.foreach(_.toThrift.write(oproto))
      oproto.writeListEnd()
      transport.getArray()
    } catch {
      case NonFatal(e) => errorReceiver.counter(e.getClass.getName).incr(); return Future.Unit
    }

    val request = Request(Method.Post, "/api/v1/spans")
    request.headerMap.add("Host", hostname)
    request.headerMap.add("Content-Type", "application/x-thrift")
    request.headerMap.add("Content-Length", serializedSpans.length.toString)
    request.content =  Buf.ByteArray.Owned(serializedSpans)

    Trace.letClear { // Don't recurse tracing by tracing sending spans to the collector.
      client(request)
        .onSuccess(_ => okCounter.incr())
        .rescue {
          case NonFatal(e) => errorReceiver.counter(e.getClass.getName).incr(); Future.Done
        }.unit
    }
  }

https://github.com/openzipkin/zipkin/blob/scala/zipkin-web/src/main/scala/com/twitter/finagle/zipkin/thrift/HttpZipkinTracer.scala
https://github.com/openzipkin/zipkin/blob/scala/zipkin-web/src/test/scala/com/twitter/zipkin/web/HttpZipkinTracerTest.scala

[codefromthecrypt]

cc @kristofa I know y'all at SoundCloud are using zipkin, though possibly more interested in kafka

[kevinoliver]

@adriancole seems reasonable. It'd probably need a bit of work because finagle-zipkin brings in the thrift tracer via service loading.

To do it in a backwards compatible manner, I think that implies moving most of everything in finagle-zipkin into a finagle-zipkin-core and then leaving the service loader in finagle-zipkin. Then you can add a new module finagle-zipkin-http that depends on finagle-zipkin-core and finagle-http and adds this tracer.

But maybe I'm jumping to conclusions with regards to what you are thinking from the service loader aspect.

[codefromthecrypt]

Thanks for having a look, Kevin. so addressing service loader might undo the current hack of DefaultTracer.self = HttpZipkinTracer or similar, I guess. One thing about the http tracer is that unlike scribe, there's no sensible localhost default. If using service loader, it would need to I suppose resolve a name from discovery, else they'd still need to explicitly configure it? Or maybe I'm missing something. Let's bear in mind that kafka transport will likely follow.

[kevinoliver]

Ok. So if since it needs configuration, this can be done manually as you mentioned — DefaultTracer.self = YourHttpTracerThatYouveConfigured. Another approach is to use service loading plus flags (e.g. System properties).

I would defer to you and your users on how you'd prefer to do it. I suspect the former is simpler for most though.

Kafka sounds like it would be similar, there'd be a new finagle-zipkin-kafka module, and it could be configured in whatever fashion is desired.

[codefromthecrypt]

Another approach is to use service loading plus flags (e.g. System
properties).
Thanks for the idea. Do you literally mean System.getProperty or Twitter
flags? Do you have an example inside finagle somewhere of someone
idiomatically using flags for a service loader implementation? I'm happy to
write this, just curious the idiomatic way.

I would defer to you and your users on how you'd prefer to do it. I
suspect the former is simpler for most though.
Yeah it makes it easier although default would probably need to be
localhost (which would be OK for those in dev or forwarding http traffic)

Kafka sounds like it would be similar, there'd be a new
finagle-zipkin-kafka module, and it could be configured in whatever fashion
is desired.
Gotcha. Once I write and get the HTTP one in the right shape for merge,
Kafka should be easy.

[kristofa]

@adriancole Thanks for taking this up and moving http and kafka support in Finagle!

At SoundCloud we have wrapper classes for building Finagle servers and clients. These thin wrappers deal with configuration like settings up the tracer. So instead of relying on service loading or overriding the DefaultTracer we explicitly set the tracer we use like this:

Http.server
      .configured(param.Tracer(tracer))
      ...

[codefromthecrypt]

At SoundCloud we have wrapper classes for building Finagle servers and
clients. These thin wrappers deal with configuration like settings up the
tracer. So instead of relying on service loading or overriding the
DefaultTracer we explicitly set the tracer we use like this:

Http.server
.configured(param.Tracer(tracer))
...

Sidebar, but I think failures in finding all the places to do ^^ resulted
in my discovery of DefaultTracer. Probably I was missing some subtle
entrypoint that wasn't configured via stack I guess. Sledgehammer, but
DefaultTracer works for everything :P I suspect the best place to handle
this is in the README of the resulting tracer. Ex suggest the stacks
approach you mentioned (with a fallback of DefaultTracer.self if that
doesn't have the intended effect)

[kevinoliver]

Thanks for the idea. Do you literally mean System.getProperty or Twitter
flags? Do you have an example inside finagle somewhere of someone
idiomatically using flags for a service loader implementation? I'm happy to
write this, just curious the idiomatic way.

I think a c.t.app.GlobalFlag is fine. In practice users can pass a value for the GlobalFlag via a System.property so it works and you also get the command-line documentation from flags. finagle-stats does this — it comes in via server loader and then some of its configuration is done via flags. See useCounterDeltas for example.

Re @kristofa's comments — explicit configuration is nice. I think @adriancole is probably also correct that it won't catch everything, but I think it practice its close to everything.

[codefromthecrypt]

ps there's some related work about abstracting the reporting function generically here. This would apply to kafka, too, for example: https://github.com/openzipkin/zipkin-java/issues/181

[codefromthecrypt]

FYI, if someone wants to make an interim kafka subtype of RawZipkinTracer, it would look something like this..

--snip--
  val topic = "zipkin"
  val kafka = "localhost:9092"
  val props: Properties = new Properties
  props.put("bootstrap.servers", kafka)
  props.put("metadata.broker.list", kafka)
  props.put("serializer.class", "kafka.serializer.DefaultEncoder")
  props.put("producer.type", "sync")
  props.put("request.required.acks", "1")
  val producer = new Producer[Array[Byte], Array[Byte]](new ProducerConfig(props))

  override def logSpans(spans: Seq[Span]): Future[Unit] = {
    // Write spans to a thrift list used as a keyed message body
    val zipkinSpans = try {
      val transport = new TMemoryBuffer(0)
      val oproto = new TBinaryProtocol(transport)
      oproto.writeListBegin(new TList(TType.STRUCT, spans.size))
      spans.map(_.toThrift).foreach(_.write(oproto))
      oproto.writeListEnd()
      transport.getArray
    } catch {
      case NonFatal(e) => errorReceiver.counter(e.getClass.getName).incr(); return Future.Unit
    }

    // send the message off, incrementing counters as needed
    try {
      producer.send(new KeyedMessage(topic, zipkinSpans))
      okCounter.incr()
    } catch {
      case NonFatal(e) => errorReceiver.counter(e.getClass.getName).incr();
    }
    return Future.Unit
  }

[codefromthecrypt]

fyi I plan to start on this tomorrow

[codefromthecrypt]

@kevinoliver so here's what I am thinking, which I think is exactly what you said a while back:

finagle-zipkin remains the same, basically it holds what we might otherwise call finagle-zipkin-scribe

• This allows folks to be undisrupted from a service loader POV. ie. they get scribe still with same flags
• This is changed insofar as it depends on a new module finagle-zipkin-core, which pulls out the non-scribe code

finagle-zipkin-http and -kafka hold reporters for each of the transport, as well global flags to control the endpoints, and service loader definitions to enable them

[sveinnfannar]

Hi guys, I started some work on this over the weekend. I just synced with @adriancole and I'll take this over.

[codefromthecrypt]

Looks like the first wave of this work arrived in master. Thanks for the progress folks!

[codefromthecrypt]

zipkin-finagle has all three transports implemented. please try it out!

https://github.com/openzipkin/zipkin-finagle
https://github.com/openzipkin/zipkin-finagle-example