Fix setting internal status code in jaeger receivers (#1105)

Bug description: if "http.status_code" and "error" tags are both set in incoming a jaeger span, internal status code is incorrectly set to Unknown code ignoring value from http.status_code tag. This commit fixes the described bug, so http.status_code tag always has precedence over error tag during Jaeger -> Internal data model translation.
open-telemetry · Jun 11, 2020 · 876cf09 · 876cf09
1 parent a71185a
commit 876cf09
Show file tree

Hide file tree

Showing 2 changed files with 136 additions and 24 deletions.
diff --git a/translator/trace/jaeger/jaegerproto_to_traces.go b/translator/trace/jaeger/jaegerproto_to_traces.go
@@ -208,42 +208,39 @@ func jTagsToInternalAttributes(tags []model.KeyValue, dest pdata.AttributeMap) {
 func setInternalSpanStatus(attrs pdata.AttributeMap, dest pdata.SpanStatus) {
 	dest.InitEmpty()
 
-	var codeSet bool
-	if codeAttr, ok := attrs.Get(tracetranslator.TagStatusCode); ok {
-		code, err := getStatusCodeFromAttr(codeAttr)
-		if err == nil {
-			codeSet = true
-			dest.SetCode(code)
-			attrs.Delete(tracetranslator.TagStatusCode)
-		}
-	} else if errorVal, ok := attrs.Get(tracetranslator.TagError); ok {
+	statusCode := pdata.StatusCode(otlptrace.Status_Ok)
+
+	if errorVal, ok := attrs.Get(tracetranslator.TagError); ok {
 		if errorVal.BoolVal() {
-			dest.SetCode(pdata.StatusCode(otlptrace.Status_UnknownError))
-			codeSet = true
+			statusCode = pdata.StatusCode(otlptrace.Status_UnknownError)
+			attrs.Delete(tracetranslator.TagError)
 		}
 	}
 
-	if codeSet {
+	if codeAttr, ok := attrs.Get(tracetranslator.TagStatusCode); ok {
+		if code, err := getStatusCodeFromAttr(codeAttr); err == nil {
+			statusCode = code
+			attrs.Delete(tracetranslator.TagStatusCode)
+		}
 		if msgAttr, ok := attrs.Get(tracetranslator.TagStatusMsg); ok {
 			dest.SetMessage(msgAttr.StringVal())
 			attrs.Delete(tracetranslator.TagStatusMsg)
 		}
-		attrs.Delete(tracetranslator.TagError)
-	} else {
-		httpCodeAttr, ok := attrs.Get(tracetranslator.TagHTTPStatusCode)
-		if ok {
-			code, err := getStatusCodeFromHTTPStatusAttr(httpCodeAttr)
-			if err == nil {
-				dest.SetCode(code)
+	} else if httpCodeAttr, ok := attrs.Get(tracetranslator.TagHTTPStatusCode); ok {
+		if code, err := getStatusCodeFromHTTPStatusAttr(httpCodeAttr); err == nil {
+
+			// Do not set status code to OK in case it was set to Unknown based on "error" tag
+			if code != pdata.StatusCode(otlptrace.Status_Ok) {
+				statusCode = code
+			}
+
+			if msgAttr, ok := attrs.Get(tracetranslator.TagHTTPStatusMsg); ok {
+				dest.SetMessage(msgAttr.StringVal())
 			}
-		} else {
-			dest.SetCode(pdata.StatusCode(otlptrace.Status_Ok))
-		}
-		if msgAttr, ok := attrs.Get(tracetranslator.TagHTTPStatusMsg); ok {
-			dest.SetMessage(msgAttr.StringVal())
 		}
 	}
 
+	dest.SetCode(statusCode)
 }
 
 func getStatusCodeFromAttr(attrVal pdata.AttributeValue) (pdata.StatusCode, error) {

diff --git a/translator/trace/jaeger/jaegerproto_to_traces_test.go b/translator/trace/jaeger/jaegerproto_to_traces_test.go
@@ -244,6 +244,121 @@ func TestProtoBatchToInternalTraces(t *testing.T) {
 	}
 }
 
+func TestSetInternalSpanStatus(t *testing.T) {
+
+	okStatus := pdata.NewSpanStatus()
+	okStatus.InitEmpty()
+	okStatus.SetCode(pdata.StatusCode(otlptrace.Status_Ok))
+
+	unknownStatus := pdata.NewSpanStatus()
+	unknownStatus.InitEmpty()
+	unknownStatus.SetCode(pdata.StatusCode(otlptrace.Status_UnknownError))
+
+	canceledStatus := pdata.NewSpanStatus()
+	canceledStatus.InitEmpty()
+	canceledStatus.SetCode(pdata.StatusCode(otlptrace.Status_Cancelled))
+
+	invalidStatusWithMessage := pdata.NewSpanStatus()
+	invalidStatusWithMessage.InitEmpty()
+	invalidStatusWithMessage.SetCode(pdata.StatusCode(otlptrace.Status_InvalidArgument))
+	invalidStatusWithMessage.SetMessage("Error: Invalid argument")
+
+	notFoundStatus := pdata.NewSpanStatus()
+	notFoundStatus.InitEmpty()
+	notFoundStatus.SetCode(pdata.StatusCode(otlptrace.Status_NotFound))
+
+	notFoundStatusWithMessage := pdata.NewSpanStatus()
+	notFoundStatusWithMessage.InitEmpty()
+	notFoundStatusWithMessage.SetCode(pdata.StatusCode(otlptrace.Status_NotFound))
+	notFoundStatusWithMessage.SetMessage("HTTP 404: Not Found")
+
+	tests := []struct {
+		name             string
+		attrs            pdata.AttributeMap
+		status           pdata.SpanStatus
+		attrsModifiedLen int // Length of attributes map after dropping converted fields
+	}{
+		{
+			name:             "No tags set -> OK status",
+			attrs:            pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{}),
+			status:           okStatus,
+			attrsModifiedLen: 0,
+		},
+		{
+			name: "error tag set -> Unknown status",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagError: pdata.NewAttributeValueBool(true),
+			}),
+			status:           unknownStatus,
+			attrsModifiedLen: 0,
+		},
+		{
+			name: "status.code is set as int",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagStatusCode: pdata.NewAttributeValueInt(1),
+			}),
+			status:           canceledStatus,
+			attrsModifiedLen: 0,
+		},
+		{
+			name: "status.code, status.message and error tags are set",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagError:      pdata.NewAttributeValueBool(true),
+				tracetranslator.TagStatusCode: pdata.NewAttributeValueInt(3),
+				tracetranslator.TagStatusMsg:  pdata.NewAttributeValueString("Error: Invalid argument"),
+			}),
+			status:           invalidStatusWithMessage,
+			attrsModifiedLen: 0,
+		},
+		{
+			name: "http.status_code tag is set as string",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagHTTPStatusCode: pdata.NewAttributeValueString("404"),
+			}),
+			status:           notFoundStatus,
+			attrsModifiedLen: 1,
+		},
+		{
+			name: "http.status_code, http.status_message and error tags are set",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagError:          pdata.NewAttributeValueBool(true),
+				tracetranslator.TagHTTPStatusCode: pdata.NewAttributeValueInt(404),
+				tracetranslator.TagHTTPStatusMsg:  pdata.NewAttributeValueString("HTTP 404: Not Found"),
+			}),
+			status:           notFoundStatusWithMessage,
+			attrsModifiedLen: 2,
+		},
+		{
+			name: "status.code has precedence over http.status_code.",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagStatusCode:     pdata.NewAttributeValueInt(1),
+				tracetranslator.TagHTTPStatusCode: pdata.NewAttributeValueInt(500),
+				tracetranslator.TagHTTPStatusMsg:  pdata.NewAttributeValueString("Server Error"),
+			}),
+			status:           canceledStatus,
+			attrsModifiedLen: 2,
+		},
+		{
+			name: "Ignore http.status_code == 200 if error set to true.",
+			attrs: pdata.NewAttributeMap().InitFromMap(map[string]pdata.AttributeValue{
+				tracetranslator.TagError:          pdata.NewAttributeValueBool(true),
+				tracetranslator.TagHTTPStatusCode: pdata.NewAttributeValueInt(200),
+			}),
+			status:           unknownStatus,
+			attrsModifiedLen: 1,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			status := pdata.NewSpanStatus()
+			setInternalSpanStatus(test.attrs, status)
+			assert.EqualValues(t, test.status, status)
+			assert.Equal(t, test.attrsModifiedLen, test.attrs.Len())
+		})
+	}
+}
+
 func TestProtoBatchesToInternalTraces(t *testing.T) {
 	batches := []*model.Batch{
 		{