From 3a63e9fc2535f042ea7ec781cde53a112205d863 Mon Sep 17 00:00:00 2001 From: Benedikt Bongartz Date: Tue, 11 Oct 2022 20:28:58 +0200 Subject: [PATCH 1/2] [pkg/translator/jaeger] in 4xx range span status MUST be left unset in case of server kind Signed-off-by: Benedikt Bongartz --- .../jaeger/jaegerproto_to_traces.go | 24 +++++++++++--- .../jaeger/jaegerproto_to_traces_test.go | 32 ++++++++++++++++--- .../jaeger/jaegerthrift_to_traces.go | 2 +- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/pkg/translator/jaeger/jaegerproto_to_traces.go b/pkg/translator/jaeger/jaegerproto_to_traces.go index 71b6b07cf9d8..006e6300957a 100644 --- a/pkg/translator/jaeger/jaegerproto_to_traces.go +++ b/pkg/translator/jaeger/jaegerproto_to_traces.go @@ -222,7 +222,7 @@ func jSpanToInternal(span *model.Span, dest ptrace.Span) { attrs := dest.Attributes() attrs.EnsureCapacity(len(span.Tags)) jTagsToInternalAttributes(span.Tags, attrs) - setInternalSpanStatus(attrs, dest.Status()) + setInternalSpanStatus(attrs, dest) if spanKindAttr, ok := attrs.Get(tracetranslator.TagSpanKind); ok { dest.SetKind(jSpanKindToInternal(spanKindAttr.Str())) attrs.Remove(tracetranslator.TagSpanKind) @@ -258,12 +258,13 @@ func jTagsToInternalAttributes(tags []model.KeyValue, dest pcommon.Map) { } } -func setInternalSpanStatus(attrs pcommon.Map, dest ptrace.Status) { +func setInternalSpanStatus(attrs pcommon.Map, span ptrace.Span) { + dest := span.Status() statusCode := ptrace.StatusCodeUnset statusMessage := "" statusExists := false - if errorVal, ok := attrs.Get(tracetranslator.TagError); ok { + if errorVal, ok := attrs.Get(tracetranslator.TagError); ok && errorVal.Type() == pcommon.ValueTypeBool { if errorVal.Bool() { statusCode = ptrace.StatusCodeError attrs.Remove(tracetranslator.TagError) @@ -302,7 +303,7 @@ func setInternalSpanStatus(attrs pcommon.Map, dest ptrace.Status) { // Fallback to introspecting if this span represents a failed HTTP // request or response, but again, only do so if the `error` tag was // not set to true and no explicit status was sent. - if code, err := getStatusCodeFromHTTPStatusAttr(httpCodeAttr); err == nil { + if code, err := getStatusCodeFromHTTPStatusAttr(httpCodeAttr, span.Kind()); err == nil { if code != ptrace.StatusCodeUnset { statusExists = true statusCode = code @@ -353,12 +354,25 @@ func codeFromAttr(attrVal pcommon.Value) (int64, error) { return val, nil } -func getStatusCodeFromHTTPStatusAttr(attrVal pcommon.Value) (ptrace.StatusCode, error) { +func getStatusCodeFromHTTPStatusAttr(attrVal pcommon.Value, kind ptrace.SpanKind) (ptrace.StatusCode, error) { statusCode, err := codeFromAttr(attrVal) if err != nil { return ptrace.StatusCodeUnset, err } + // For HTTP status codes in the 4xx range span status MUST be left unset + // in case of SpanKind.SERVER and MUST be set to Error in case of SpanKind.CLIENT. + // For HTTP status codes in the 5xx range, as well as any other code the client + // failed to interpret, span status MUST be set to Error. + if statusCode >= 400 && statusCode < 500 { + switch kind { + case ptrace.SpanKindClient: + return ptrace.StatusCodeError, nil + case ptrace.SpanKindServer: + return ptrace.StatusCodeUnset, nil + } + } + return tracetranslator.StatusCodeFromHTTP(statusCode), nil } diff --git a/pkg/translator/jaeger/jaegerproto_to_traces_test.go b/pkg/translator/jaeger/jaegerproto_to_traces_test.go index 135cfb4bb8e7..1174de85c6ab 100644 --- a/pkg/translator/jaeger/jaegerproto_to_traces_test.go +++ b/pkg/translator/jaeger/jaegerproto_to_traces_test.go @@ -93,41 +93,52 @@ func TestGetStatusCodeFromHTTPStatusAttr(t *testing.T) { tests := []struct { name string attr pcommon.Value + kind ptrace.SpanKind code ptrace.StatusCode }{ { name: "string-unknown", attr: pcommon.NewValueStr("10"), + kind: ptrace.SpanKindClient, code: ptrace.StatusCodeError, }, { name: "string-ok", attr: pcommon.NewValueStr("101"), + kind: ptrace.SpanKindClient, code: ptrace.StatusCodeUnset, }, { name: "int-not-found", attr: pcommon.NewValueInt(404), + kind: ptrace.SpanKindClient, code: ptrace.StatusCodeError, }, + { + name: "int-not-found-client-span", + attr: pcommon.NewValueInt(404), + kind: ptrace.SpanKindServer, + code: ptrace.StatusCodeUnset, + }, { name: "int-invalid-arg", attr: pcommon.NewValueInt(408), + kind: ptrace.SpanKindClient, code: ptrace.StatusCodeError, }, - { name: "int-internal", attr: pcommon.NewValueInt(500), + kind: ptrace.SpanKindClient, code: ptrace.StatusCodeError, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - code, err := getStatusCodeFromHTTPStatusAttr(test.attr) + code, err := getStatusCodeFromHTTPStatusAttr(test.attr, test.kind) assert.NoError(t, err) assert.Equal(t, test.code, code) }) @@ -346,6 +357,7 @@ func TestSetInternalSpanStatus(t *testing.T) { name string attrs map[string]interface{} status ptrace.Status + kind ptrace.SpanKind attrsModifiedLen int // Length of attributes map after dropping converted fields }{ { @@ -416,14 +428,26 @@ func TestSetInternalSpanStatus(t *testing.T) { status: errorStatus, attrsModifiedLen: 1, }, + { + name: "the 4xx range span status MUST be left unset in case of SpanKind.SERVER", + kind: ptrace.SpanKindServer, + attrs: map[string]interface{}{ + tracetranslator.TagError: false, + conventions.AttributeHTTPStatusCode: 404, + }, + status: emptyStatus, + attrsModifiedLen: 2, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - status := ptrace.NewStatus() + span := ptrace.NewSpan() + span.SetKind(test.kind) + status := span.Status() attrs := pcommon.NewMap() attrs.FromRaw(test.attrs) - setInternalSpanStatus(attrs, status) + setInternalSpanStatus(attrs, span) assert.EqualValues(t, test.status, status) assert.Equal(t, test.attrsModifiedLen, attrs.Len()) }) diff --git a/pkg/translator/jaeger/jaegerthrift_to_traces.go b/pkg/translator/jaeger/jaegerthrift_to_traces.go index d0a743151414..f82bb9da9618 100644 --- a/pkg/translator/jaeger/jaegerthrift_to_traces.go +++ b/pkg/translator/jaeger/jaegerthrift_to_traces.go @@ -106,7 +106,7 @@ func jThriftSpanToInternal(span *jaeger.Span, dest ptrace.Span) { attrs := dest.Attributes() attrs.EnsureCapacity(len(span.Tags)) jThriftTagsToInternalAttributes(span.Tags, attrs) - setInternalSpanStatus(attrs, dest.Status()) + setInternalSpanStatus(attrs, dest) if spanKindAttr, ok := attrs.Get(tracetranslator.TagSpanKind); ok { dest.SetKind(jSpanKindToInternal(spanKindAttr.Str())) attrs.Remove(tracetranslator.TagSpanKind) From 0c992c131c7f2d26e63f51c5da91d4a01e958865 Mon Sep 17 00:00:00 2001 From: Benedikt Bongartz Date: Wed, 19 Oct 2022 18:31:26 +0200 Subject: [PATCH 2/2] [pkg/translator/jaeger] provide changelog Signed-off-by: Benedikt Bongartz --- ...uscode_unset_in_case_of_spankind_server_error.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100755 .chloggen/let_statuscode_unset_in_case_of_spankind_server_error.yaml diff --git a/.chloggen/let_statuscode_unset_in_case_of_spankind_server_error.yaml b/.chloggen/let_statuscode_unset_in_case_of_spankind_server_error.yaml new file mode 100755 index 000000000000..156df66bfe28 --- /dev/null +++ b/.chloggen/let_statuscode_unset_in_case_of_spankind_server_error.yaml @@ -0,0 +1,11 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: jaegertranslator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER and MUST be set to Error in case of SpanKind.CLIENT. + +# One or more tracking issues related to the change +issues: [8273]