Extract transport agnostic zipkin code into finagle-zipkin-core #513

sveinnfannar · 2016-05-31T16:32:39Z

Problem

Address issue #465 (Add http and kafka transports to zipkin-tracer).
Zipkin can receive traces via Kafka, HTTP and Scribe. Some Finagle users (including Quizup) already using Kafka are reluctant to introduce Scribe into their stack as it is no longer maintained.
So the purpose of this PR is to support Kafka as a transport for Zipkin traces, as well as structuring the code such that it allows other transports to be added (like HTTP).

Solution

Extract most of finagle-zipkin into finagle-zipkin-core, leaving finagle-zipkin with only scribe specific stuff (with no api breaking changes to finagle-zipkin of course). ~~Then implement finagle-zipkin-kafka, which depends on finagle-zipkin-core~~. Kafka transport will be introduced in a separate PR.

This structure was initially suggested by @kevinoliver in #465 (comment).

Todo

Extract common code into finagle-zipkin-core (largest part)
~~Implement finagle-zipkin-kafka (in progress)~~ (separate PR)
Clean up Pants and SBT dependencies

Questions

@mosesn can you check this out and provide some feedback?
@adriancole is this in line with what you had envisioned?

codefromthecrypt · 2016-06-01T01:51:36Z

finagle-zipkin-core/src/main/scala/com/twitter/finagle/zipkin/core/SamplingTracer.scala

@@ -0,0 +1,160 @@
+package com.twitter.finagle.zipkin.core


weird this didn't get caught as a rename..

yes, it did in my local git diff

btw, I fixed up the commits so this appears as a rename

codefromthecrypt · 2016-06-01T01:57:00Z

added a couple notes, none of which are important :) carry on. looks like the right path

codefromthecrypt · 2016-06-01T01:57:59Z

finagle-zipkin/src/main/scala/com/twitter/finagle/zipkin/thrift/ScribeRawZipkinTracer.scala

+import scala.collection.mutable.ArrayBuffer
+import scala.language.reflectiveCalls
+
+object ScribeRawZipkinTracer {


Is there a META-INF change needed? or is the service loader unaffected by the rename

Yes it should be unaffected.
META-INF/services/com.twitter.finagle.tracing.Tracer in finagle-zipkin points to com.twitter.finagle.zipkin.thrift.SamplingTracer.

sveinnfannar · 2016-06-01T12:37:59Z

This diff is too large, I'm going to do some git foo to make it easier to review.

sveinnfannar · 2016-06-01T13:18:33Z

Ok I force pushed a change that separates the moving of files from the actual code changes. The PR is now easier to review commit by commit.

codefromthecrypt · 2016-06-01T13:51:17Z

finagle-zipkin-core/src/test/scala/com/twitter/finagle/zipkin/core/RawZipkinTracerTest.scala

-      response
+  class FakeRawZipkinTracer extends RawZipkinTracer(NullStatsReceiver) {
+    var spans: Seq[Span] = Seq.empty
+    override def sendSpans(xs: Seq[Span]): Future[Unit] = {


cool basically the extender just needs to implement sendSpans

mosesn · 2016-06-01T16:18:12Z

This looks good to me. @sveinnfannar could you add a note to CHANGES?

@sveinnfannar what do you think about splitting this up into a few PRs? when we merge it internally, it's all going to be squashed, and I think it would be nice to have separate commits for each piece. ie, the first PR would be HEAD => First Milestone, second commit would be HEAD => First Milestone => Second Milestone, etc, and for the first patch I would grab HEAD => First Milestone, second patch I would grab First Milestone => Second Milestone, etc.

I would say this first milestone is pretty close to being ready to ship.

I would also suggest breaking out the Scribe stuff to a separate module at some point in time if possible, but it's much less necessary than the work you've outlined so far.

sveinnfannar · 2016-06-01T20:20:53Z

Thanks for the review @mosesn.

Ok, I'll rename this PR and put the finishes touches on it.

Regarding breaking out the scribe stuff, do you mean something like having a module named finagle-zipkin-scribe?
finagle-zipkin already contains only scribe specific stuff. We could rename it to finagle-zipkin-scribe. Then for backwards compatibility we would have a finagle-zipkin module that just depends on finagle-zipkin-scribe.
It would basically only contain a service loader and something like this:

package com.twitter.finagle.zipkin.thrift
@deprecated("Use com.twitter.finagle.zipkin.scribe.ScribeZipkinTracer instead", "6.36.0")
class ZipkinTracer(..) extends ScribeZipkinTracer(..)

mosesn · 2016-06-01T21:00:28Z

Hmm, I thought the service-loading bit was in finagle-zipkin, but if it's somewhere else then I see what you mean. Should actually be fine this way, imo.

codefromthecrypt · 2016-06-02T02:19:08Z

+1 merging the structural changes first makes it an easy merge, and paves the way for multiple transports being reviewed or added independently (ex http, kafka)

sveinnfannar · 2016-06-02T13:51:46Z

@mosesn I finished some minor tweaks and manual testing so this PR should be ready. The notes in CHANGES still need an RB_ID. Is that something you add internally?

kevinoliver · 2016-06-02T16:05:26Z

@sveinnfannar yeah we'll add the RB_ID, thanks. i'll try to take a look today as well.

kevinoliver · 2016-06-02T16:05:57Z

CHANGES

@@ -56,6 +56,17 @@ Breaking API Changes
  * finagle-memcached: `c.t.f.memcached.Client` now uses `c.t.bijection.Bijection`
    instead of `c.t.u.Bijection`. ``RB_ID=834383``

+<<<<<<< HEAD


fyi merge conflict?

kevinoliver · 2016-06-02T16:24:46Z

@sveinnfannar this is great. my comments are all superficial and should be easy to patch up.

kevinoliver · 2016-06-02T22:30:33Z

my pleasure. let me know when its ready for another look.

sveinnfannar · 2016-06-03T12:57:09Z

@kevinoliver I addressed the comments, can you take another look?
The changes are all pretty superficial besides this one: 262614b

taylorleese · 2016-06-03T21:41:17Z

finagle-zipkin/src/main/scala/com/twitter/finagle/zipkin/thrift/ScribeRawZipkinTracer.scala

+    val transport = ClientBuilder()
+      .name("zipkin-tracer")
+      .hosts(new InetSocketAddress(scribeHost, scribePort))
+      .codec(ThriftClientFramedCodec())


@kevinoliver @mosesn Should we move this to the stack equivalent?

it should, but its not appropriate for this commit.

kevinoliver · 2016-06-03T22:49:58Z

@sveinnfannar ok looking good to me. mind fixing this round and pinging when its good?

mosesn · 2016-06-04T03:18:45Z

LGTM except for @kevinoliver's fixes

sveinnfannar · 2016-06-06T17:06:28Z

Hi @kevinoliver, I addressed your comments.
And subsequently changed to emacs since IntelliJ kept formatting random code segments like a crazy person. Sorry for all the formatting diffs.

sveinnfannar · 2016-06-06T17:08:41Z

@adriancole, I'm putting the finishing touches on the kafka PR, stay tuned :)

kevinoliver · 2016-06-06T17:22:48Z

cool @sveinnfannar looks good now. i'll work on getting this merged in. thanks.

kevinoliver · 2016-06-06T17:38:02Z

hmm @sveinnfannar i'm running into some conflicts applying the commits locally. do you mind rebasing on develop and then squashing these down to a single commit?

sveinnfannar · 2016-06-06T17:47:01Z

Yes no problem

Move most of finagle-zipkin to finagle-zipkin-core Pants build files for finagle-zipkin-core Add finagle-zipkin-core as a dependency of finagle-zipkin Separate scribe specific code Fix non-exhaustive case warning Move SamplingTracer to finagle-zipkin-core Generic tracer cache with flush on shutdown Remove scribe specific tests already in finagle-zipkin and add test for flushing tracers Fix import Weed out sbt and pants dependencies Make internal classes private Updated CHANGES, change still needs a RB_ID Improved scaladoc comments Annotate return types where needed Remove confusing SamplingTracer from finagle-zipkin Minor dependency fix Fixes after code review Fix some more IntelliJ formatting fuckups Fix after failed rebase

sveinnfannar · 2016-06-06T18:15:19Z

@kevinoliver done 👍

kevinoliver · 2016-06-07T00:10:55Z

ok, this one has decent sized work on our end to get it integrated. i'll keep at it but it may take a few days.

sveinnfannar · 2016-06-07T09:40:27Z

ok, just ping me on gitter if I can do anything to help

kevinoliver · 2016-06-08T04:10:14Z

@sveinnfannar i made good progress on this today and should get it merged in by the end of the week.

kevinoliver · 2016-06-09T23:17:58Z

alrighty! got this merged in locally and should show up on the develop branch this coming monday.

thanks for contribution.

sveinnfannar · 2016-06-13T10:43:25Z

Great to hear. Thanks for the help.

roanta · 2016-06-21T16:59:19Z

Thanks! This was merged and landed: 989edc5

codefromthecrypt reviewed Jun 1, 2016
View reviewed changes

sveinnfannar force-pushed the develop branch from da68e7c to ec454e8 Compare June 1, 2016 13:15

codefromthecrypt reviewed Jun 1, 2016
View reviewed changes

sveinnfannar changed the title ~~WIP: Support Kafka transport for Zipkin traces~~ WIP: Extract transport agnostic zipkin code into finagle-zipkin-core Jun 1, 2016

sveinnfannar changed the title ~~WIP: Extract transport agnostic zipkin code into finagle-zipkin-core~~ Extract transport agnostic zipkin code into finagle-zipkin-core Jun 2, 2016

kevinoliver reviewed Jun 2, 2016
View reviewed changes

taylorleese reviewed Jun 3, 2016
View reviewed changes

sveinnfannar force-pushed the develop branch from 32b3692 to 18156b5 Compare June 6, 2016 16:59

kevinoliver added the Ship It label Jun 6, 2016

sveinnfannar force-pushed the develop branch from 18156b5 to 968189b Compare June 6, 2016 18:12

sveinnfannar force-pushed the develop branch from 968189b to 04d7d27 Compare June 6, 2016 18:14

sveinnfannar mentioned this pull request Jun 7, 2016

Kafka transport for Zipkin traces #515

Closed

kevinoliver added the Merged Internally label Jun 9, 2016

roanta closed this Jun 21, 2016

Extract transport agnostic zipkin code into finagle-zipkin-core #513

Extract transport agnostic zipkin code into finagle-zipkin-core #513

Conversation

sveinnfannar commented May 31, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveinnfannar commented Jun 1, 2016

sveinnfannar commented Jun 1, 2016

Choose a reason for hiding this comment

mosesn commented Jun 1, 2016

sveinnfannar commented Jun 1, 2016

mosesn commented Jun 1, 2016

codefromthecrypt commented Jun 2, 2016 via email

sveinnfannar commented Jun 2, 2016

kevinoliver commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinoliver commented Jun 2, 2016

kevinoliver commented Jun 2, 2016

sveinnfannar commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinoliver commented Jun 3, 2016

mosesn commented Jun 4, 2016

sveinnfannar commented Jun 6, 2016

sveinnfannar commented Jun 6, 2016

kevinoliver commented Jun 6, 2016

kevinoliver commented Jun 6, 2016

sveinnfannar commented Jun 6, 2016

sveinnfannar commented Jun 6, 2016

kevinoliver commented Jun 7, 2016

sveinnfannar commented Jun 7, 2016

kevinoliver commented Jun 8, 2016

kevinoliver commented Jun 9, 2016

sveinnfannar commented Jun 13, 2016

roanta commented Jun 21, 2016

sveinnfannar commented May 31, 2016 •

edited