Merge pull request #758 from openzipkin/less-magical-json

Removes implicit type of numeric binary annotations

zipkin-common/src/main/scala/com/twitter/zipkin/json/JsonAnnotation.scala

@@ -6,8 +6,8 @@ case class JsonAnnotation(timestamp: Long, value: String, endpoint: Option[JsonE
 
 object JsonAnnotation extends (Annotation => JsonAnnotation) {
   override def apply(a: Annotation) =
-    JsonAnnotation(a.timestamp, a.value, a.host.map(JsonService))
+    JsonAnnotation(a.timestamp, a.value, a.host.map(JsonEndpoint))
 
   def invert(a: JsonAnnotation) =
-    Annotation(a.timestamp, a.value, a.endpoint.map(JsonService.invert))
+    Annotation(a.timestamp, a.value, a.endpoint.map(JsonEndpoint.invert))
 }

zipkin-common/src/main/scala/com/twitter/zipkin/json/JsonBinaryAnnotation.scala

@@ -25,30 +25,26 @@ object JsonBinaryAnnotation extends (BinaryAnnotation => JsonBinaryAnnotation) {
         case Bool.value => (None, if (b.value.get() != 0) true else false)
         case Bytes.value => (Some("BYTES"), base64.encode(b.value.array(), b.value.position(), b.value.remaining()))
         case I16.value => (Some("I16"), b.value.getShort)
-        case I32.value => (None, b.value.getInt)
+        case I32.value => (Some("I32"), b.value.getInt)
         case I64.value => (Some("I64"), b.value.getLong)
-        case Double.value => (None, b.value.getDouble)
+        case Double.value => (Some("DOUBLE"), b.value.getDouble)
         case String.value => (None, new String(b.value.array(), b.value.position(), b.value.remaining(), Utf8))
         case _ => throw new Exception("Unsupported annotation type: %s".format(b))
       }
     } catch {
       case e: Exception => "Error parsing binary annotation: %s".format(exceptionString(e))
     }
-    JsonBinaryAnnotation(b.key, value, annotationType, b.host.map(JsonService))
+    JsonBinaryAnnotation(b.key, value, annotationType, b.host.map(JsonEndpoint))
   }
 
   def invert(b: JsonBinaryAnnotation) = {
     val annotationType = b.annotationType
       .map(upperCamel.convert(_))
       .map(AnnotationType.fromName(_))
       .getOrElse(b.value match {
-      // annotationType is mostly redundant in json, especially as most annotations are strings
-      // knowing this is json, the only way this won't map is list or map
+      // The only json types that can be implicit are booleans and strings. Numbers vary in shape.
       case bool: Boolean => Bool
-      // Jackson defaults floats to doubles, and zipkin thrift doesn't have a float type
-      case double: Double => Double
       case string: String => String
-      case number: Number => I32 // default numeric is I32
       case _ => throw new IllegalArgumentException("Unsupported json annotation type: %s".format(b))
     })
 
@@ -66,7 +62,7 @@ object JsonBinaryAnnotation extends (BinaryAnnotation => JsonBinaryAnnotation) {
     } catch {
       case e: Exception => ByteBuffer.wrap("Error parsing json binary annotation: %s".format(exceptionString(e)).getBytes(Utf8))
     }
-    new BinaryAnnotation(b.key, bytes, annotationType, b.endpoint.map(JsonService.invert))
+    new BinaryAnnotation(b.key, bytes, annotationType, b.endpoint.map(JsonEndpoint.invert))
   }
 
   private[this] def exceptionString(e: Exception) =

zipkin-common/src/main/scala/com/twitter/zipkin/json/JsonEndpoint.scala

@@ -10,7 +10,7 @@ import com.twitter.zipkin.common.Endpoint
  */
 case class JsonEndpoint(serviceName: String, ipv4: String, port: Option[Int])
 
-object JsonService extends (Endpoint => JsonEndpoint) {
+object JsonEndpoint extends (Endpoint => JsonEndpoint) {
   override def apply(e: Endpoint) =
     new JsonEndpoint(e.serviceName, e.getHostAddress, if (e.port == 0) None else Some(e.getUnsignedPort))
 

zipkin-common/src/main/scala/com/twitter/zipkin/json/ZipkinJson.scala

@@ -10,7 +10,7 @@ import com.twitter.zipkin.common._
 
 object ZipkinJson extends ObjectMapper with ScalaObjectMapper {
   val module = new SimpleModule("ZipkinJson")
-    .addSerializer(classOf[Endpoint], serializer(JsonService))
+    .addSerializer(classOf[Endpoint], serializer(JsonEndpoint))
     .addSerializer(classOf[Annotation], serializer(JsonAnnotation))
     .addSerializer(classOf[BinaryAnnotation], serializer(JsonBinaryAnnotation))
     .addSerializer(classOf[Span], serializer(JsonSpan))

zipkin-common/src/test/scala/com/twitter/zipkin/json/JsonConversionTest.scala

@@ -12,15 +12,15 @@ class JsonConversionTest extends FunSuite with Matchers {
   test("endpoint") {
     val convert = JsonEndpoint("zipkin-query", "192.168.0.1", Some(9411))
 
-    assert(JsonService(endpoint) == convert)
-    JsonService(JsonService.invert(convert)) should be(convert)
+    assert(JsonEndpoint(endpoint) == convert)
+    JsonEndpoint(JsonEndpoint.invert(convert)) should be(convert)
   }
 
   test("endpoint without port") {
     val convert = JsonEndpoint("zipkin-query", "192.168.0.1", None)
 
-    assert(JsonService(endpoint.copy(port = 0)) == convert)
-    JsonService(JsonService.invert(convert)) should be(convert)
+    assert(JsonEndpoint(endpoint.copy(port = 0)) == convert)
+    JsonEndpoint(JsonEndpoint.invert(convert)) should be(convert)
   }
 
   test("bytes") {
@@ -87,22 +87,6 @@ class JsonConversionTest extends FunSuite with Matchers {
     JsonBinaryAnnotation(JsonBinaryAnnotation.invert(unqualified)) should be(unqualified)
   }
 
-  test("int's annotation type is implicit") {
-    val unqualified = JsonBinaryAnnotation("key", 6, None, None)
-    val qualified = JsonBinaryAnnotation("key", 6, Some("I32"), None)
-
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(qualified)) should be(unqualified)
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(unqualified)) should be(unqualified)
-  }
-
-  test("double's annotation type is implicit") {
-    val unqualified = JsonBinaryAnnotation("key", 6.0D, None, None)
-    val qualified = JsonBinaryAnnotation("key", 6.0D, Some("DOUBLE"), None)
-
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(qualified)) should be(unqualified)
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(unqualified)) should be(unqualified)
-  }
-
   test("string's annotation type is implicit") {
     val unqualified = JsonBinaryAnnotation("key", "HELLO!", None, None)
     val qualified = JsonBinaryAnnotation("key", "HELLO!", Some("STRING"), None)
@@ -111,12 +95,10 @@ class JsonConversionTest extends FunSuite with Matchers {
     JsonBinaryAnnotation(JsonBinaryAnnotation.invert(unqualified)) should be(unqualified)
   }
 
-  test("float coerses to double type in json") { // since there's no FLOAT type
-    val unqualified = JsonBinaryAnnotation("key", 6.0, None, None)
-    val qualified = JsonBinaryAnnotation("key", 6.0, Some("DOUBLE"), None)
-    val double = JsonBinaryAnnotation("key", 6.0D, None, None)
+  test("float coerses to double type in json") { // since there's no FLOAT type in the thrift
+    val float = JsonBinaryAnnotation("key", 6.0, Some("DOUBLE"), None)
+    val double = JsonBinaryAnnotation("key", 6.0D, Some("DOUBLE"), None)
 
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(qualified)) should be(double)
-    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(unqualified)) should be(double)
+    JsonBinaryAnnotation(JsonBinaryAnnotation.invert(float)) should be(double)
   }
 }

zipkin-common/src/test/scala/com/twitter/zipkin/json/ZipkinJsonTest.scala

@@ -1,12 +1,11 @@
 package com.twitter.zipkin.json
 
-import java.nio.ByteBuffer
-
 import com.google.common.base.Charsets.UTF_8
 import com.twitter.util.Time
 import com.twitter.zipkin.Constants
 import com.twitter.zipkin.common._
 import org.scalatest.{FunSuite, Matchers}
+import java.nio.ByteBuffer
 
 /**
  * Tests that who how data is serialized, so that subtle code changes don't break users.
@@ -194,15 +193,6 @@ class ZipkinJsonTest extends FunSuite with Matchers {
     )
   }
 
-  test("binary annotation: I32 doesn't need type") {
-    val a = BinaryAnnotation("someKey", ByteBuffer.wrap(Array[Byte](1, 1, 1, 1)), AnnotationType.I32, None)
-    assert(mapper.writeValueAsString(a) ==
-      """
-        |{"key":"someKey","value":16843009}
-      """.stripMargin.trim
-    )
-  }
-
   test("binary annotation: I64 adds type") {
     val a = BinaryAnnotation("someKey", ByteBuffer.allocate(8).putLong(0, 1234L), AnnotationType.I64, None)
     assert(mapper.writeValueAsString(a) ==
@@ -211,14 +201,4 @@ class ZipkinJsonTest extends FunSuite with Matchers {
       """.stripMargin.trim
     )
   }
-
-
-  test("binary annotation: Double doesn't need type") {
-    val a = BinaryAnnotation("someKey", ByteBuffer.allocate(8).putDouble(0, 1.1D), AnnotationType.Double, None)
-    assert(mapper.writeValueAsString(a) ==
-      """
-        |{"key":"someKey","value":1.1}
-      """.stripMargin.trim
-    )
-  }
 }

zipkin-web/src/main/scala/com/twitter/zipkin/web/Handlers.scala

@@ -310,7 +310,7 @@ class Handlers(mustacheGenerator: ZipkinMustache, queryExtractor: QueryExtractor
         case ann if ZConstants.CoreAddress.contains(ann.key) =>
           val key = ZConstants.CoreAnnotationNames.get(ann.key).get
           val value = ann.host.map { e => s"${e.getHostAddress}:${e.getUnsignedPort}" }.get
-          JsonBinaryAnnotation(key, value, None, ann.host.map(JsonService))
+          JsonBinaryAnnotation(key, value, None, ann.host.map(JsonEndpoint))
         case ann => JsonBinaryAnnotation(ann)
       }