Prefer BinaryAnnotation.host over Zipkin "lc" annotation for service name (#394)

Per zipkinCore.Thrift comments for "lc" (LOCAL_COMPONENT) it should be used to provide the "component" of a local span, however, the service context should come from BinnaryAnnotation.host. The recent changes were overwriting binary annotation host information if the "lc" was available, however, "lc" should be the last resort. This change only uses "lc" if no endpoint or binary annotation host is available.

Fix #393

Author: Paulo Janotti

translator/trace/testdata/zipkin_v1_local_component.json

@@ -13,5 +13,25 @@
                 "value": "myLocalComponent"
             }
         ]
+    },
+    {
+        "traceId": "0ed2e63cbe71f5a8",
+        "name": "sendOrder",
+        "id": "fe351a053fbcac2f",
+        "parentId": "0ed2e63cbe71f5a8",
+        "timestamp": 1544805927453925,
+        "duration": 3740,
+        "annotations": [],
+        "binaryAnnotations": [
+            {
+                "key": "lc",
+                "value": "myLocalComponent",
+                "endpoint": {
+                    "ipv4": "172.31.0.7",
+                    "port": 0,
+                    "serviceName": "myServiceName"
+                }
+            }
+        ]
     }
 ]

translator/trace/testdata/zipkin_v1_thrift_local_component.json

@@ -14,5 +14,26 @@
                 "value": "bXlMb2NhbENvbXBvbmVudA=="
             }
         ]
+    },
+    {
+        "trace_id": 1068169210207794600,
+        "name": "sendOrder",
+        "id": -129168404463703007,
+        "parent_id": 1068169210207794600,
+        "timestamp": 1544805927453925,
+        "duration": 3740,
+        "annotations": [],
+        "binary_annotations": [
+            {
+                "key": "lc",
+                "annotation_type": "STRING",
+                "value": "bXlMb2NhbENvbXBvbmVudA==",
+                "host": {
+                    "ipv4": -1407254521,
+                    "port": 0,
+                    "service_name": "myServiceName"
+                }
+            }
+        ]
     }
 ]

translator/trace/zipkinv1_thrift_to_protospan.go

@@ -137,12 +137,12 @@ func toTranslatorEndpoint(e *zipkincore.Endpoint) *endpoint {
 
 var trueByteSlice = []byte{1}
 
-func zipkinV1ThriftBinAnnotationsToOCAttributes(ztBinAnnotations []*zipkincore.BinaryAnnotation) (attributes *tracepb.Span_Attributes, localComponent string) {
+func zipkinV1ThriftBinAnnotationsToOCAttributes(ztBinAnnotations []*zipkincore.BinaryAnnotation) (attributes *tracepb.Span_Attributes, fallbackServiceName string) {
 	if len(ztBinAnnotations) == 0 {
 		return nil, ""
 	}
 
-	var fallbackServiceName string
+	var localComponent string
 	attributeMap := make(map[string]*tracepb.AttributeValue)
 	for _, binaryAnnotation := range ztBinAnnotations {
 		pbAttrib := &tracepb.AttributeValue{}
@@ -200,14 +200,14 @@ func zipkinV1ThriftBinAnnotationsToOCAttributes(ztBinAnnotations []*zipkincore.B
 		attributeMap[key] = pbAttrib
 	}
 
-	if localComponent == "" && fallbackServiceName != "" {
-		localComponent = fallbackServiceName
+	if fallbackServiceName == "" {
+		fallbackServiceName = localComponent
 	}
 
 	attributes = &tracepb.Span_Attributes{
 		AttributeMap: attributeMap,
 	}
-	return attributes, localComponent
+	return attributes, fallbackServiceName
 }
 
 var errNotEnoughBytes = errors.New("not enough bytes representing the number")

translator/trace/zipkinv1_thrift_to_protospan_test.go

@@ -39,8 +39,20 @@ func TestZipkinThriftFallbackToLocalComponent(t *testing.T) {
 		t.Fatalf("failed to translate zipkinv1 thrift to OC proto: %v", err)
 	}
 
+	if len(reqs) != 2 {
+		t.Fatalf("got %d trace service request(s), want 2", len(reqs))
+	}
+
+	// First span didn't have a host/endpoint to give service name, use the local component.
 	got := reqs[0].Node.ServiceInfo.Name
-	const want = "myLocalComponent"
+	want := "myLocalComponent"
+	if got != want {
+		t.Fatalf("got %q for service name, want %q", got, want)
+	}
+
+	// Second span have a host/endpoint to give service name, do not use local component.
+	got = reqs[1].Node.ServiceInfo.Name
+	want = "myServiceName"
 	if got != want {
 		t.Fatalf("got %q for service name, want %q", got, want)
 	}

translator/trace/zipkinv1_to_protospan.go

@@ -175,12 +175,12 @@ func zipkinV1ToOCSpan(zSpan *zipkinV1Span) (*tracepb.Span, *annotationParseResul
 	return ocSpan, parsedAnnotations, nil
 }
 
-func zipkinV1BinAnnotationsToOCAttributes(binAnnotations []*binaryAnnotation) (attributes *tracepb.Span_Attributes, localComponent string) {
+func zipkinV1BinAnnotationsToOCAttributes(binAnnotations []*binaryAnnotation) (attributes *tracepb.Span_Attributes, fallbackServiceName string) {
 	if len(binAnnotations) == 0 {
 		return nil, ""
 	}
 
-	var fallbackServiceName string
+	var localComponent string
 	attributeMap := make(map[string]*tracepb.AttributeValue)
 	for _, binAnnotation := range binAnnotations {
 		if binAnnotation.Endpoint != nil && binAnnotation.Endpoint.ServiceName != "" {
@@ -209,15 +209,15 @@ func zipkinV1BinAnnotationsToOCAttributes(binAnnotations []*binaryAnnotation) (a
 		return nil, ""
 	}
 
-	if localComponent == "" && fallbackServiceName != "" {
-		localComponent = fallbackServiceName
+	if fallbackServiceName == "" {
+		fallbackServiceName = localComponent
 	}
 
 	attributes = &tracepb.Span_Attributes{
 		AttributeMap: attributeMap,
 	}
 
-	return attributes, localComponent
+	return attributes, fallbackServiceName
 }
 
 // annotationParseResult stores the results of examining the original annotations,

translator/trace/zipkinv1_to_protospan_test.go

@@ -141,8 +141,20 @@ func TestZipkinJSONFallbackToLocalComponent(t *testing.T) {
 		t.Fatalf("failed to translate zipkinv1 to OC proto: %v", err)
 	}
 
+	if len(reqs) != 2 {
+		t.Fatalf("got %d trace service request(s), want 2", len(reqs))
+	}
+
+	// First span didn't have a host/endpoint to give service name, use the local component.
 	got := reqs[0].Node.ServiceInfo.Name
-	const want = "myLocalComponent"
+	want := "myLocalComponent"
+	if got != want {
+		t.Fatalf("got %q for service name, want %q", got, want)
+	}
+
+	// Second span have a host/endpoint to give service name, do not use local component.
+	got = reqs[1].Node.ServiceInfo.Name
+	want = "myServiceName"
 	if got != want {
 		t.Fatalf("got %q for service name, want %q", got, want)
 	}