Remove ParentSpanID from domain model (#831)

* Remove ParentSpanID from domain model

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Add missing tests

Signed-off-by: Yuri Shkuro <ys@uber.com>

cmd/collector/app/metrics.go

@@ -141,7 +141,7 @@ func (m metricsBySvc) ReportServiceNameForSpan(span *model.Span) {
 	if span.Flags.IsDebug() {
 		m.countDebugSpansByServiceName(serviceName)
 	}
-	if span.ParentSpanID == 0 {
+	if span.ParentSpanID() == 0 {
 		m.countTracesByServiceName(serviceName)
 	}
 }

cmd/collector/app/metrics_test.go

@@ -43,7 +43,7 @@ func TestProcessorMetrics(t *testing.T) {
 	}
 	jFormat.ReceivedBySvc.ReportServiceNameForSpan(&mSpan)
 	mSpan.Flags.SetDebug()
-	mSpan.ParentSpanID = model.SpanID(1234)
+	mSpan.ReplaceParentID(1234)
 	jFormat.ReceivedBySvc.ReportServiceNameForSpan(&mSpan)
 	counters, gauges := baseMetrics.LocalBackend.Snapshot()
 

model/adjuster/clockskew.go

@@ -115,14 +115,14 @@ func (a *clockSkewAdjuster) buildSubGraphs() {
 	a.roots = make(map[model.SpanID]*node)
 	for _, n := range a.spans {
 		// TODO handle FOLLOWS_FROM references
-		if n.span.ParentSpanID == 0 {
+		if n.span.ParentSpanID() == 0 {
 			a.roots[n.span.SpanID] = n
 			continue
 		}
-		if p, ok := a.spans[n.span.ParentSpanID]; ok {
+		if p, ok := a.spans[n.span.ParentSpanID()]; ok {
 			p.children = append(p.children, n)
 		} else {
-			warning := fmt.Sprintf(warningFormatInvalidParentID, n.span.ParentSpanID)
+			warning := fmt.Sprintf(warningFormatInvalidParentID, n.span.ParentSpanID())
 			n.span.Warnings = append(n.span.Warnings, warning)
 			// Treat spans with invalid parent ID as root spans
 			a.roots[n.span.SpanID] = n

model/adjuster/clockskew_test.go

@@ -53,13 +53,14 @@ func TestClockSkewAdjuster(t *testing.T) {
 					Fields:    []model.KeyValue{model.String("event", "some event")},
 				})
 			}
+			traceID := model.TraceID{Low: 1}
 			span := &model.Span{
-				TraceID:      model.TraceID{Low: 1},
-				SpanID:       model.SpanID(spanProto.id),
-				ParentSpanID: model.SpanID(spanProto.parent),
-				StartTime:    toTime(spanProto.startTime),
-				Duration:     toDuration(spanProto.duration),
-				Logs:         logs,
+				TraceID:    traceID,
+				SpanID:     model.SpanID(spanProto.id),
+				References: []model.SpanRef{model.NewChildOfRef(traceID, model.SpanID(spanProto.parent))},
+				StartTime:  toTime(spanProto.startTime),
+				Duration:   toDuration(spanProto.duration),
+				Logs:       logs,
 				Process: &model.Process{
 					ServiceName: spanProto.host,
 					Tags: []model.KeyValue{

model/adjuster/span_id_deduper.go

@@ -82,7 +82,7 @@ func (d *spanIDDeduper) dedupeSpanIDs() {
 				continue
 			}
 			oldToNewSpanIDs[span.SpanID] = newID
-			span.ParentSpanID = span.SpanID // previously shared ID is the new parent
+			span.ReplaceParentID(span.SpanID) // previously shared ID is the new parent
 			span.SpanID = newID
 		}
 	}
@@ -93,9 +93,9 @@ func (d *spanIDDeduper) dedupeSpanIDs() {
 // spans whose IDs we deduped.
 func (d *spanIDDeduper) swapParentIDs(oldToNewSpanIDs map[model.SpanID]model.SpanID) {
 	for _, span := range d.trace.Spans {
-		if parentID, ok := oldToNewSpanIDs[span.ParentSpanID]; ok {
+		if parentID, ok := oldToNewSpanIDs[span.ParentSpanID()]; ok {
 			if span.SpanID != parentID {
-				span.ParentSpanID = parentID
+				span.ReplaceParentID(parentID)
 			}
 		}
 	}

model/adjuster/span_id_deduper_test.go

@@ -29,28 +29,32 @@ const (
 )
 
 func newTrace() *model.Trace {
+	traceID := model.TraceID{Low: 42}
 	return &model.Trace{
 		Spans: []*model.Span{
 			{
 				// client span
-				SpanID: clientSpanID,
+				TraceID: traceID,
+				SpanID:  clientSpanID,
 				Tags: model.KeyValues{
 					// span.kind = client
 					model.String(string(ext.SpanKind), string(ext.SpanKindRPCClientEnum)),
 				},
 			},
 			{
 				// server span
-				SpanID: clientSpanID, // shared span ID
+				TraceID: traceID,
+				SpanID:  clientSpanID, // shared span ID
 				Tags: model.KeyValues{
 					// span.kind = server
 					model.String(string(ext.SpanKind), string(ext.SpanKindRPCServerEnum)),
 				},
 			},
 			{
 				// some other span, child of server span
-				SpanID:       anotherSpanID,
-				ParentSpanID: clientSpanID,
+				TraceID:    traceID,
+				SpanID:     anotherSpanID,
+				References: []model.SpanRef{model.NewChildOfRef(traceID, clientSpanID)},
 			},
 		},
 	}
@@ -67,11 +71,11 @@ func TestSpanIDDeduperTriggered(t *testing.T) {
 
 	serverSpan := trace.Spans[1]
 	assert.Equal(t, clientSpanID+1, serverSpan.SpanID, "server span ID should be reassigned")
-	assert.Equal(t, clientSpan.SpanID, serverSpan.ParentSpanID, "client span should be server span's parent")
+	assert.Equal(t, clientSpan.SpanID, serverSpan.ParentSpanID(), "client span should be server span's parent")
 
 	thirdSpan := trace.Spans[2]
 	assert.Equal(t, anotherSpanID, thirdSpan.SpanID, "3rd span ID should not change")
-	assert.Equal(t, serverSpan.SpanID, thirdSpan.ParentSpanID, "server span should be 3rd span's parent")
+	assert.Equal(t, serverSpan.SpanID, thirdSpan.ParentSpanID(), "server span should be 3rd span's parent")
 }
 
 func TestSpanIDDeduperNotTriggered(t *testing.T) {
@@ -88,7 +92,7 @@ func TestSpanIDDeduperNotTriggered(t *testing.T) {
 
 	thirdSpan := trace.Spans[1]
 	assert.Equal(t, anotherSpanID, thirdSpan.SpanID, "3rd span ID should not change")
-	assert.Equal(t, serverSpan.SpanID, thirdSpan.ParentSpanID, "server span should be 3rd span's parent")
+	assert.Equal(t, serverSpan.SpanID, thirdSpan.ParentSpanID(), "server span should be 3rd span's parent")
 }
 
 func TestSpanIDDeduperError(t *testing.T) {

model/converter/json/fixtures/domain_01.json

@@ -70,7 +70,13 @@
     {
       "traceID": "1",
       "spanID": "3",
-      "parentSpanID": "2",
+      "references": [
+          {
+              "refType": "child-of",
+              "traceID": "1",
+              "spanID": "2"
+          }
+      ],
       "operationName": "some-operation",
       "startTime": "2017-01-26T16:46:31.639875139-05:00",
       "duration": 5000,
@@ -111,7 +117,6 @@
     {
       "traceID": "1",
       "spanID": "5",
-      "parentSpanID": "4",
       "operationName": "preserveParentID-test",
       "references": [
         {

model/converter/json/fixtures/domain_es_01.json

@@ -1,23 +1,22 @@
 {
   "traceID": "1",
   "spanID": "2",
-  "parentSpanID": "3",
   "operationName": "test-general-conversion",
   "references": [
-    {
-      "refType": "child-of",
-      "traceID": "ff",
-      "spanID": "ff"
-    },
     {
       "refType": "child-of",
       "traceID": "1",
-      "spanID": "2"
+      "spanID": "3"
     },
     {
       "refType": "follows-from",
       "traceID": "1",
-      "spanID": "2"
+      "spanID": "4"
+    },
+    {
+      "refType": "child-of",
+      "traceID": "ff",
+      "spanID": "ff"
     }
   ],
   "flags": 1,

model/converter/json/fixtures/es_01.json

@@ -1,24 +1,23 @@
 {
   "traceID": "1",
   "spanID": "2",
-  "parentSpanID": "3",
   "flags": 1,
   "operationName": "test-general-conversion",
   "references": [
-    {
-      "refType": "CHILD_OF",
-      "traceID": "ff",
-      "spanID": "ff"
-    },
     {
       "refType": "CHILD_OF",
       "traceID": "1",
-      "spanID": "2"
+      "spanID": "3"
     },
     {
       "refType": "FOLLOWS_FROM",
       "traceID": "1",
-      "spanID": "2"
+      "spanID": "4"
+    },
+    {
+      "refType": "CHILD_OF",
+      "traceID": "ff",
+      "spanID": "ff"
     }
   ],
   "startTime": 1485467191639875,

model/converter/json/from_domain.go

@@ -77,45 +77,26 @@ func (fd fromDomain) convertSpan(span *model.Span, processID json.ProcessID) jso
 	s := fd.convertSpanInternal(span)
 	s.ProcessID = processID
 	s.Warnings = span.Warnings
-	s.References = fd.convertReferences(span, false)
+	s.References = fd.convertReferences(span)
 	return s
 }
 
 func (fd fromDomain) convertSpanEmbedProcess(span *model.Span) *json.Span {
 	s := fd.convertSpanInternal(span)
 	process := fd.convertProcess(span.Process)
 	s.Process = &process
-	s.ParentSpanID = json.SpanID(span.ParentSpanID.String())
-	s.References = fd.convertReferences(span, true)
+	s.References = fd.convertReferences(span)
 	return &s
 }
 
-// when preserveParentID==false the parent ID is converted to a CHILD_OF reference
-func (fd fromDomain) convertReferences(span *model.Span, preserveParentID bool) []json.Reference {
-	length := len(span.References)
-	if span.ParentSpanID != 0 && !preserveParentID {
-		length++
-	}
-	out := make([]json.Reference, 0, length)
-	var parentRefAdded bool
+func (fd fromDomain) convertReferences(span *model.Span) []json.Reference {
+	out := make([]json.Reference, 0, len(span.References))
 	for _, ref := range span.References {
 		out = append(out, json.Reference{
 			RefType: fd.convertRefType(ref.RefType),
 			TraceID: json.TraceID(ref.TraceID.String()),
 			SpanID:  json.SpanID(ref.SpanID.String()),
 		})
-		if ref.TraceID == span.TraceID && ref.SpanID == span.ParentSpanID {
-			parentRefAdded = true
-		}
-	}
-	if span.ParentSpanID != 0 && !preserveParentID && !parentRefAdded {
-		// By this point, if ParentSpanID != 0 but there are no other references,
-		// then the ParentSpanID does refer to child-of type
-		out = append(out, json.Reference{
-			RefType: json.ChildOf,
-			TraceID: json.TraceID(span.TraceID.String()),
-			SpanID:  json.SpanID(span.ParentSpanID.String()),
-		})
 	}
 	return out
 }

model/converter/json/from_domain_test.go

@@ -33,13 +33,13 @@ const NumberOfFixtures = 1
 
 func TestFromDomain(t *testing.T) {
 	for i := 1; i <= NumberOfFixtures; i++ {
-		inStr, outStr := testReadFixtures(t, i, false)
+		domainStr, jsonStr := testReadFixtures(t, i, false)
 
 		var trace model.Trace
-		require.NoError(t, json.Unmarshal(inStr, &trace))
+		require.NoError(t, json.Unmarshal(domainStr, &trace))
 		uiTrace := FromDomain(&trace)
 
-		testOutput(t, i, outStr, uiTrace, false)
+		testOutput(t, i, jsonStr, uiTrace, false)
 	}
 	// this is just to confirm the uint64 representation of float64(72.5) used as a "temperature" tag
 	assert.Equal(t, int64(4634802150889750528), int64(math.Float64bits(72.5)))
@@ -60,6 +60,7 @@ func TestFromDomainEmbedProcess(t *testing.T) {
 	}
 }
 
+// Loads and returns domain model and JSON model fixtures with given number i.
 func testReadFixtures(t *testing.T, i int, processEmbedded bool) ([]byte, []byte) {
 	var in string
 	if processEmbedded {
@@ -80,7 +81,7 @@ func testReadFixtures(t *testing.T, i int, processEmbedded bool) ([]byte, []byte
 	return inStr, outStr
 }
 
-func testOutput(t *testing.T, i int, outStr []byte, object interface{}, processEmbedded bool) {
+func testOutput(t *testing.T, i int, expectedStr []byte, object interface{}, processEmbedded bool) {
 	buf := &bytes.Buffer{}
 	enc := json.NewEncoder(buf)
 	enc.SetIndent("", "  ")
@@ -94,7 +95,7 @@ func testOutput(t *testing.T, i int, outStr []byte, object interface{}, processE
 		require.NoError(t, enc.Encode(object.(*jModel.Trace)))
 	}
 
-	if !assert.Equal(t, string(outStr), string(buf.Bytes())) {
+	if !assert.Equal(t, string(expectedStr), string(buf.Bytes())) {
 		err := ioutil.WriteFile(outFile+"-actual.json", buf.Bytes(), 0644)
 		assert.NoError(t, err)
 	}

model/converter/json/to_domain.go

@@ -58,15 +58,18 @@ func (td toDomain) spanToDomain(dbSpan *json.Span) (*model.Span, error) {
 	if err != nil {
 		return nil, err
 	}
-	parentSpanIDInt, err := model.SpanIDFromString(string(dbSpan.ParentSpanID))
-	if err != nil {
-		return nil, err
+
+	if dbSpan.ParentSpanID != "" {
+		parentSpanID, err := model.SpanIDFromString(string(dbSpan.ParentSpanID))
+		if err != nil {
+			return nil, err
+		}
+		refs = model.MaybeAddParentSpanID(traceID, parentSpanID, refs)
 	}
 
 	span := &model.Span{
 		TraceID:       traceID,
 		SpanID:        model.SpanID(spanIDInt),
-		ParentSpanID:  model.SpanID(parentSpanIDInt),
 		OperationName: dbSpan.OperationName,
 		References:    refs,
 		Flags:         model.Flags(uint32(dbSpan.Flags)),