Common refactor

- Move `Trace`, `TraceSummary` from `common` package to `query` since they are only used in the query service. - Cleanup from the move Author: @franklinhu Fixes #89 URL: #89
openzipkin · Jul 31, 2012 · 391aef2 · 391aef2
1 parent a2f42a0
commit 391aef2
Show file tree

Hide file tree

Showing 23 changed files with 103 additions and 81 deletions.
diff --git a/zipkin-common/src/main/scala/com/twitter/zipkin/adapter/Adapter.scala b/zipkin-common/src/main/scala/com/twitter/zipkin/adapter/Adapter.scala
@@ -32,7 +32,6 @@ trait Adapter {
   type binaryAnnotationType  /* corresponds to com.twitter.zipkin.common.BinaryAnnotation */
   type endpointType          /* corresponds to com.twitter.zipkin.common.Endpoint         */
   type spanType              /* corresponds to com.twitter.zipkin.common.Span             */
-  type traceSummaryType      /* corresponds to com.twitter.zipkin.common.TraceSummary     */
 
   def apply(a: annotationType): Annotation
   def apply(a: Annotation): annotationType
@@ -48,7 +47,4 @@ trait Adapter {
 
   def apply(s: spanType): Span
   def apply(s: Span): spanType
-
-  def apply(t: traceSummaryType): TraceSummary
-  def apply(t: TraceSummary): traceSummaryType
 }
diff --git a/zipkin-common/src/main/scala/com/twitter/zipkin/adapter/QueryAdapter.scala b/zipkin-common/src/main/scala/com/twitter/zipkin/adapter/QueryAdapter.scala
@@ -15,8 +15,7 @@
  */
 package com.twitter.zipkin.adapter
 
-import com.twitter.zipkin.common.Trace
-import com.twitter.zipkin.query.{TraceCombo, TraceTimeline, TimelineAnnotation}
+import com.twitter.zipkin.query._
 
 /**
  * Adapter for query related structs
@@ -25,6 +24,7 @@ trait QueryAdapter {
   type timelineAnnotationType /* corresponds to com.twitter.zipkin.query.TimelineAnnotation */
   type traceTimelineType      /* corresponds to com.twitter.zipkin.query.TraceTimeline */
   type traceComboType         /* corresponds to com.twitter.zipkin.query.TraceCombo */
+  type traceSummaryType      /* corresponds to com.twitter.zipkin.common.TraceSummary     */
   type traceType              /* corresponds to com.twitter.zipkin.query.Trace */
 
   def apply(t: timelineAnnotationType): TimelineAnnotation
@@ -36,6 +36,9 @@ trait QueryAdapter {
   def apply(t: traceComboType): TraceCombo
   def apply(t: TraceCombo): traceComboType
 
+  def apply(t: traceSummaryType): TraceSummary
+  def apply(t: TraceSummary): traceSummaryType
+
   def apply(t: traceType): Trace
   def apply(t: Trace): traceType
 }
diff --git a/...ala/com/twitter/zipkin/common/Trace.scala → ...cala/com/twitter/zipkin/query/Trace.scala b/...ala/com/twitter/zipkin/common/Trace.scala → ...cala/com/twitter/zipkin/query/Trace.scala
@@ -13,13 +13,13 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package com.twitter.zipkin.common
+package com.twitter.zipkin.query
 
+import com.twitter.finagle.tracing.{Trace => FTrace}
 import com.twitter.logging.Logger
+import com.twitter.zipkin.common.{BinaryAnnotation, Endpoint, Span}
 import java.nio.ByteBuffer
 import scala.collection.mutable
-import com.twitter.finagle.tracing.{Trace => FTrace}
-import com.twitter.zipkin.query.SpanTreeEntry
 
 /**
  * A chunk of time, between a start and an end.

diff --git a/zipkin-common/src/main/scala/com/twitter/zipkin/query/TraceCombo.scala b/zipkin-common/src/main/scala/com/twitter/zipkin/query/TraceCombo.scala
@@ -15,8 +15,6 @@
  */
 package com.twitter.zipkin.query
 
-import com.twitter.zipkin.common.{Trace, TraceSummary}
-
 object TraceCombo {
   def apply(trace: Trace): TraceCombo = {
     TraceCombo(trace, TraceSummary(trace), TraceTimeline(trace), trace.toSpanDepths)

diff --git a/.../twitter/zipkin/common/TraceSummary.scala → ...m/twitter/zipkin/query/TraceSummary.scala b/.../twitter/zipkin/common/TraceSummary.scala → ...m/twitter/zipkin/query/TraceSummary.scala
@@ -14,9 +14,10 @@
  *  limitations under the License.
  *
  */
-package com.twitter.zipkin.common
+package com.twitter.zipkin.query
 
 import scala.collection.Map
+import com.twitter.zipkin.common.Endpoint
 
 object TraceSummary {
 

diff --git a/zipkin-common/src/main/scala/com/twitter/zipkin/query/TraceTimeline.scala b/zipkin-common/src/main/scala/com/twitter/zipkin/query/TraceTimeline.scala
@@ -15,7 +15,7 @@
  */
 package com.twitter.zipkin.query
 
-import com.twitter.zipkin.common.{Trace, Endpoint, BinaryAnnotation}
+import com.twitter.zipkin.common.{Endpoint, BinaryAnnotation}
 
 object TraceTimeline {
   def apply(trace: Trace): Option[TraceTimeline] = {

diff --git a/zipkin-finatra/src/main/scala/com/twitter/zipkin/adapter/JsonAdapter.scala b/zipkin-finatra/src/main/scala/com/twitter/zipkin/adapter/JsonAdapter.scala
@@ -1,7 +1,7 @@
 package com.twitter.zipkin.adapter
 
 import com.twitter.zipkin.common._
-import com.twitter.zipkin.common.json.{JsonTraceSummary, JsonBinaryAnnotation, JsonSpan}
+import com.twitter.zipkin.common.json.{JsonBinaryAnnotation, JsonSpan}
 
 /**
  * Adapter to make common classes compatible with Jackson/Jerkson JSON generation
@@ -12,7 +12,6 @@ object JsonAdapter extends Adapter {
   type binaryAnnotationType = JsonBinaryAnnotation
   type endpointType = Endpoint
   type spanType = JsonSpan
-  type traceSummaryType = JsonTraceSummary
 
   /* No change between JSON and common types */
   def apply(a: annotationType): Annotation = a
@@ -61,14 +60,6 @@ object JsonAdapter extends Adapter {
       s.annotations.sortWith((a,b) => a.timestamp < b.timestamp),
       s.binaryAnnotations.map(JsonAdapter(_)))
   }
-
-  def apply(t: traceSummaryType): TraceSummary = {
-    TraceSummary(t.traceId.toLong, t.startTimestamp, t.endTimestamp, t.durationMicro, t.serviceCounts, t.endpoints)
-  }
-
-  def apply(t: TraceSummary): traceSummaryType =  {
-    JsonTraceSummary(t.traceId.toString, t.startTimestamp, t.endTimestamp, t.durationMicro, t.serviceCounts.toMap, t.endpoints)
-  }
 }
 
 
diff --git a/zipkin-finatra/src/main/scala/com/twitter/zipkin/adapter/JsonQueryAdapter.scala b/zipkin-finatra/src/main/scala/com/twitter/zipkin/adapter/JsonQueryAdapter.scala
@@ -1,8 +1,7 @@
 package com.twitter.zipkin.adapter
 
-import com.twitter.zipkin.common.Trace
-import com.twitter.zipkin.common.json.{JsonTimelineAnnotation, JsonTrace, JsonTraceCombo, JsonTraceTimeline}
-import com.twitter.zipkin.query.{TraceCombo, TraceTimeline, TimelineAnnotation}
+import com.twitter.zipkin.common.json._
+import com.twitter.zipkin.query._
 
 /**
  * JS doesn't like Longs so we need to convert them to strings
@@ -11,6 +10,7 @@ object JsonQueryAdapter extends QueryAdapter {
   type timelineAnnotationType = JsonTimelineAnnotation
   type traceTimelineType = JsonTraceTimeline
   type traceComboType = JsonTraceCombo
+  type traceSummaryType = JsonTraceSummary
   type traceType = JsonTrace
 
   /* no change between json and common */
@@ -44,7 +44,7 @@ object JsonQueryAdapter extends QueryAdapter {
   def apply(t: traceComboType): TraceCombo = {
     TraceCombo(
       JsonQueryAdapter(t.trace),
-      t.traceSummary.map(JsonAdapter(_)),
+      t.traceSummary.map(JsonQueryAdapter(_)),
       t.traceTimeline.map(JsonQueryAdapter(_)),
       t.spanDepths)
   }
@@ -53,11 +53,19 @@ object JsonQueryAdapter extends QueryAdapter {
   def apply(t: TraceCombo): traceComboType = {
     JsonTraceCombo(
       JsonQueryAdapter(t.trace),
-      t.traceSummary.map(JsonAdapter(_)),
+      t.traceSummary.map(JsonQueryAdapter(_)),
       t.traceTimeline.map(JsonQueryAdapter(_)),
       t.spanDepths)
   }
 
+  def apply(t: traceSummaryType): TraceSummary = {
+    TraceSummary(t.traceId.toLong, t.startTimestamp, t.endTimestamp, t.durationMicro, t.serviceCounts, t.endpoints)
+  }
+
+  def apply(t: TraceSummary): traceSummaryType =  {
+    JsonTraceSummary(t.traceId.toString, t.startTimestamp, t.endTimestamp, t.durationMicro, t.serviceCounts.toMap, t.endpoints)
+  }
+
   /* json to common */
   def apply(t: traceType): Trace = {
     throw new Exception("NOT IMPLEMENTED")

diff --git a/zipkin-finatra/src/main/scala/com/twitter/zipkin/common/json/JsonTraceSummary.scala b/zipkin-finatra/src/main/scala/com/twitter/zipkin/common/json/JsonTraceSummary.scala
@@ -1,6 +1,7 @@
 package com.twitter.zipkin.common.json
 
-import com.twitter.zipkin.common.{Endpoint, TraceSummary}
+import com.twitter.zipkin.common.Endpoint
+import com.twitter.zipkin.query.TraceSummary
 
 object JsonTraceSummary {
   def apply(t: TraceSummary): JsonTraceSummary =

diff --git a/zipkin-finatra/src/main/scala/com/twitter/zipkin/web/App.scala b/zipkin-finatra/src/main/scala/com/twitter/zipkin/web/App.scala
@@ -95,7 +95,7 @@ class App(config: ZipkinWebConfig, client: gen.ZipkinQuery.FinagledClient) exten
         case _ => {
           client.getTraceSummariesByIds(ids, adjusters).map {
             _.map { summary =>
-              JsonAdapter(ThriftAdapter(summary))
+              JsonQueryAdapter(ThriftQueryAdapter(summary))
             }
           }
         }

diff --git a/zipkin-scrooge/src/main/scala/com/twitter/zipkin/adapter/ThriftAdapter.scala b/zipkin-scrooge/src/main/scala/com/twitter/zipkin/adapter/ThriftAdapter.scala
@@ -16,9 +16,9 @@
  */
 package com.twitter.zipkin.adapter
 
+import com.twitter.util.Duration
 import com.twitter.zipkin.gen
 import com.twitter.zipkin.common._
-import com.twitter.util.Duration
 import java.util.concurrent.TimeUnit
 
 object ThriftAdapter extends Adapter {
@@ -27,7 +27,6 @@ object ThriftAdapter extends Adapter {
   type binaryAnnotationType = gen.BinaryAnnotation
   type endpointType = gen.Endpoint
   type spanType = gen.Span
-  type traceSummaryType = gen.TraceSummary
 
   /* Annotation from Thrift */
   def apply(a: annotationType): Annotation = {
@@ -116,17 +115,4 @@ object ThriftAdapter extends Adapter {
     gen.Span(s.traceId, s.name, s.id, s.parentId, s.annotations.map { this(_) },
       s.binaryAnnotations.map { this(_) }, s.debug)
   }
-
-  /* TraceSummary from Thrift */
-  def apply(t: traceSummaryType): TraceSummary = {
-    new TraceSummary(t.traceId, t.startTimestamp, t.endTimestamp,
-      t.durationMicro, t.serviceCounts,
-      t.endpoints.map(ThriftAdapter(_)).toList)
-  }
-
-  /* TraceSummary to Thrift */
-  def apply(t: TraceSummary): traceSummaryType = {
-    gen.TraceSummary(t.traceId, t.startTimestamp, t.endTimestamp,
-      t.durationMicro, t.serviceCounts, t.endpoints.map(ThriftAdapter(_)))
-  }
 }
diff --git a/zipkin-scrooge/src/main/scala/com/twitter/zipkin/adapter/ThriftQueryAdapter.scala b/zipkin-scrooge/src/main/scala/com/twitter/zipkin/adapter/ThriftQueryAdapter.scala
@@ -16,13 +16,13 @@
 package com.twitter.zipkin.adapter
 
 import com.twitter.zipkin.gen
-import com.twitter.zipkin.query.{TraceCombo, TraceTimeline, TimelineAnnotation}
-import com.twitter.zipkin.common.Trace
+import com.twitter.zipkin.query._
 
 object ThriftQueryAdapter extends QueryAdapter {
   type timelineAnnotationType = gen.TimelineAnnotation
   type traceTimelineType = gen.TraceTimeline
   type traceComboType = gen.TraceCombo
+  type traceSummaryType = gen.TraceSummary
   type traceType = gen.Trace
 
   /* TimelineAnnotation from Thrift */
@@ -71,7 +71,7 @@ object ThriftQueryAdapter extends QueryAdapter {
   def apply(t: traceComboType): TraceCombo = {
     TraceCombo(
       ThriftQueryAdapter(t.`trace`),
-      t.`summary`.map(ThriftAdapter(_)),
+      t.`summary`.map(ThriftQueryAdapter(_)),
       t.`timeline`.map(ThriftQueryAdapter(_)),
       t.`spanDepths`.map(_.toMap))
   }
@@ -80,11 +80,24 @@ object ThriftQueryAdapter extends QueryAdapter {
   def apply(t: TraceCombo): traceComboType = {
     gen.TraceCombo(
       ThriftQueryAdapter(t.trace),
-      t.traceSummary.map(ThriftAdapter(_)),
+      t.traceSummary.map(ThriftQueryAdapter(_)),
       t.traceTimeline.map(ThriftQueryAdapter(_)),
       t.spanDepths)
   }
 
+  /* TraceSummary from Thrift */
+  def apply(t: traceSummaryType): TraceSummary = {
+    new TraceSummary(t.traceId, t.startTimestamp, t.endTimestamp,
+      t.durationMicro, t.serviceCounts,
+      t.endpoints.map(ThriftAdapter(_)).toList)
+  }
+
+  /* TraceSummary to Thrift */
+  def apply(t: TraceSummary): traceSummaryType = {
+    gen.TraceSummary(t.traceId, t.startTimestamp, t.endTimestamp,
+      t.durationMicro, t.serviceCounts, t.endpoints.map(ThriftAdapter(_)))
+  }
+
   /* Trace from Thrift */
   def apply(t: traceType): Trace = {
     Trace(t.`spans`.map(ThriftAdapter(_)))

diff --git a/zipkin-scrooge/src/test/scala/com/twitter/zipkin/adapter/ThriftAdapterSpec.scala b/zipkin-scrooge/src/test/scala/com/twitter/zipkin/adapter/ThriftAdapterSpec.scala
@@ -105,16 +105,5 @@ class ThriftAdapterSpec extends Specification with JMocker with ClassMocker {
         ThriftAdapter(noBinaryAnnotationsSpan) mustEqual Span(0, "name", 0, None, List(), Seq())
       }
     }
-
-    "convert TraceSummary" in {
-      "to thrift and back" in {
-        val expectedTraceSummary = TraceSummary(123, 10000, 10300, 300, Map("service1" -> 1),
-          List(Endpoint(123, 123, "service1")))
-        val thriftTraceSummary = ThriftAdapter(expectedTraceSummary)
-        val actualTraceSummary = ThriftAdapter(thriftTraceSummary)
-        expectedTraceSummary mustEqual actualTraceSummary
-      }
-    }
   }
-
 }
diff --git a/zipkin-scrooge/src/test/scala/com/twitter/zipkin/adapter/ThriftQueryAdapterSpec.scala b/zipkin-scrooge/src/test/scala/com/twitter/zipkin/adapter/ThriftQueryAdapterSpec.scala
@@ -0,0 +1,37 @@
+/*
+ * Copyright 2012 Twitter Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ */
+package com.twitter.zipkin.adapter
+
+import com.twitter.zipkin.common.Endpoint
+import com.twitter.zipkin.query.TraceSummary
+import org.specs.mock.{JMocker, ClassMocker}
+import org.specs.Specification
+
+class ThriftQueryAdapterSpec extends Specification with JMocker with ClassMocker {
+
+  "ThriftQueryAdapter" should {
+    "convert TraceSummary" in {
+      "to thrift and back" in {
+        val expectedTraceSummary = TraceSummary(123, 10000, 10300, 300, Map("service1" -> 1),
+          List(Endpoint(123, 123, "service1")))
+        val thriftTraceSummary = ThriftQueryAdapter(expectedTraceSummary)
+        val actualTraceSummary = ThriftQueryAdapter(thriftTraceSummary)
+        expectedTraceSummary mustEqual actualTraceSummary
+      }
+    }
+  }
+}
diff --git a/zipkin-server/src/main/scala/com/twitter/zipkin/query/QueryService.scala b/zipkin-server/src/main/scala/com/twitter/zipkin/query/QueryService.scala
@@ -23,14 +23,13 @@ import com.twitter.logging.Logger
 import com.twitter.ostrich.admin.Service
 import com.twitter.util.Future
 import com.twitter.zipkin.adapter.{ThriftQueryAdapter, ThriftAdapter}
-import com.twitter.zipkin.common.TraceSummary
 import com.twitter.zipkin.gen
 import com.twitter.zipkin.query.adjusters.Adjuster
 import com.twitter.zipkin.storage.{Aggregates, TraceIdDuration, Index, Storage}
 import java.nio.ByteBuffer
+import java.util.concurrent.atomic.AtomicBoolean
 import org.apache.thrift.TException
 import scala.collection.Set
-import java.util.concurrent.atomic.AtomicBoolean
 
 /**
  * Able to respond to users queries regarding the traces. Usually does so
@@ -183,7 +182,7 @@ class QueryService(storage: Storage, index: Index, aggregates: Aggregates, adjus
 
       storage.getTracesByIds(traceIds.toList).map { traces =>
         traces.flatMap { trace =>
-          TraceSummary(adjusters.foldLeft(trace)((t, adjuster) => adjuster.adjust(t))).map(ThriftAdapter(_))
+          TraceSummary(adjusters.foldLeft(trace)((t, adjuster) => adjuster.adjust(t))).map(ThriftQueryAdapter(_))
         }
       }
     }

diff --git a/zipkin-server/src/main/scala/com/twitter/zipkin/query/adjusters/Adjuster.scala b/zipkin-server/src/main/scala/com/twitter/zipkin/query/adjusters/Adjuster.scala
@@ -16,7 +16,7 @@
  */
 package com.twitter.zipkin.query.adjusters
 
-import com.twitter.zipkin.common.Trace
+import com.twitter.zipkin.query.Trace
 
 trait Adjuster {
 

diff --git a/zipkin-server/src/main/scala/com/twitter/zipkin/query/adjusters/TimeSkewAdjuster.scala b/zipkin-server/src/main/scala/com/twitter/zipkin/query/adjusters/TimeSkewAdjuster.scala
@@ -16,12 +16,11 @@
  */
 package com.twitter.zipkin.query.adjusters
 
+import com.twitter.finagle.tracing.{Trace => FTrace}
+import com.twitter.zipkin.common._
 import com.twitter.zipkin.gen
+import com.twitter.zipkin.query.{Trace, SpanTreeEntry}
 import scala.collection.Map
-import com.twitter.zipkin.common._
-import com.twitter.finagle.tracing.{Trace => FTrace}
-import com.twitter.zipkin.query.SpanTreeEntry
-
 
 class TimeSkewAdjuster extends Adjuster {
 

diff --git a/zipkin-server/src/main/scala/com/twitter/zipkin/storage/Storage.scala b/zipkin-server/src/main/scala/com/twitter/zipkin/storage/Storage.scala
@@ -16,8 +16,9 @@
  */
 package com.twitter.zipkin.storage
 
-import com.twitter.zipkin.common.{Trace, Span}
 import com.twitter.util.{Duration, Future}
+import com.twitter.zipkin.common.Span
+import com.twitter.zipkin.query.Trace
 
 trait Storage {