From bc648eb0b977960d200ebebb55e77bf3d285d92b Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 31 Mar 2021 13:56:26 -0700 Subject: [PATCH 1/4] Fix Jaeger span status reporting and unify tag keys Move all tag key strings to be consts defined in a unified location. Fix the status code and message tag keys to conform with the specification. Do not set the span status message if it is not set to conform with the specification. --- exporters/trace/jaeger/jaeger.go | 16 +++++--- exporters/trace/jaeger/jaeger_test.go | 56 +++++++++++++++++++++------ 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go index b1b9960da61..9ac12cc3bec 100644 --- a/exporters/trace/jaeger/jaeger.go +++ b/exporters/trace/jaeger/jaeger.go @@ -37,6 +37,10 @@ import ( const ( keyInstrumentationLibraryName = "otel.library.name" keyInstrumentationLibraryVersion = "otel.library.version" + keyError = "error" + keySpanKind = "span.kind" + keyStatusCode = "otel.status_code" + keyStatusMessage = "otel.status_description" ) type Option func(*options) @@ -269,18 +273,18 @@ func spanSnapshotToThrift(ss *export.SpanSnapshot) *gen.Span { if ss.SpanKind != trace.SpanKindInternal { tags = append(tags, - getStringTag("span.kind", ss.SpanKind.String()), + getStringTag(keySpanKind, ss.SpanKind.String()), ) } if ss.StatusCode != codes.Unset { - tags = append(tags, - getInt64Tag("status.code", int64(ss.StatusCode)), - getStringTag("status.message", ss.StatusMessage), - ) + tags = append(tags, getInt64Tag(keyStatusCode, int64(ss.StatusCode))) + if ss.StatusMessage != "" { + tags = append(tags, getStringTag(keyStatusMessage, ss.StatusMessage)) + } if ss.StatusCode == codes.Error { - tags = append(tags, getBoolTag("error", true)) + tags = append(tags, getBoolTag(keyError, true)) } } diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index a107af00af3..b7ad912d0f8 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -363,6 +363,40 @@ func Test_spanSnapshotToThrift(t *testing.T) { data *export.SpanSnapshot want *gen.Span }{ + { + name: "no status description", + data: &export.SpanSnapshot{ + SpanContext: trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + }), + Name: "/foo", + StartTime: now, + EndTime: now, + StatusCode: codes.Error, + SpanKind: trace.SpanKindClient, + InstrumentationLibrary: instrumentation.Library{ + Name: instrLibName, + Version: instrLibVersion, + }, + }, + want: &gen.Span{ + TraceIdLow: 651345242494996240, + TraceIdHigh: 72623859790382856, + SpanId: 72623859790382856, + OperationName: "/foo", + StartTime: now.UnixNano() / 1000, + Duration: 0, + Tags: []*gen.Tag{ + {Key: keyError, VType: gen.TagType_BOOL, VBool: &boolTrue}, + {Key: keyInstrumentationLibraryName, VType: gen.TagType_STRING, VStr: &instrLibName}, + {Key: keyInstrumentationLibraryVersion, VType: gen.TagType_STRING, VStr: &instrLibVersion}, + {Key: keyStatusCode, VType: gen.TagType_LONG, VLong: &statusCodeValue}, + // Should not have a status message becuase it was unset + {Key: keySpanKind, VType: gen.TagType_STRING, VStr: &spanKind}, + }, + }, + }, { name: "no parent", data: &export.SpanSnapshot{ @@ -408,12 +442,12 @@ func Test_spanSnapshotToThrift(t *testing.T) { {Key: "double", VType: gen.TagType_DOUBLE, VDouble: &doubleValue}, {Key: "key", VType: gen.TagType_STRING, VStr: &keyValue}, {Key: "int", VType: gen.TagType_LONG, VLong: &intValue}, - {Key: "error", VType: gen.TagType_BOOL, VBool: &boolTrue}, - {Key: "otel.library.name", VType: gen.TagType_STRING, VStr: &instrLibName}, - {Key: "otel.library.version", VType: gen.TagType_STRING, VStr: &instrLibVersion}, - {Key: "status.code", VType: gen.TagType_LONG, VLong: &statusCodeValue}, - {Key: "status.message", VType: gen.TagType_STRING, VStr: &statusMessage}, - {Key: "span.kind", VType: gen.TagType_STRING, VStr: &spanKind}, + {Key: keyError, VType: gen.TagType_BOOL, VBool: &boolTrue}, + {Key: keyInstrumentationLibraryName, VType: gen.TagType_STRING, VStr: &instrLibName}, + {Key: keyInstrumentationLibraryVersion, VType: gen.TagType_STRING, VStr: &instrLibVersion}, + {Key: keyStatusCode, VType: gen.TagType_LONG, VLong: &statusCodeValue}, + {Key: keyStatusMessage, VType: gen.TagType_STRING, VStr: &statusMessage}, + {Key: keySpanKind, VType: gen.TagType_STRING, VStr: &spanKind}, }, References: []*gen.SpanRef{ { @@ -486,8 +520,8 @@ func Test_spanSnapshotToThrift(t *testing.T) { Tags: []*gen.Tag{ // status code, message and span kind should NOT be populated {Key: "arr", VType: gen.TagType_STRING, VStr: &arrValue}, - {Key: "otel.library.name", VType: gen.TagType_STRING, VStr: &instrLibName}, - {Key: "otel.library.version", VType: gen.TagType_STRING, VStr: &instrLibVersion}, + {Key: keyInstrumentationLibraryName, VType: gen.TagType_STRING, VStr: &instrLibName}, + {Key: keyInstrumentationLibraryVersion, VType: gen.TagType_STRING, VStr: &instrLibVersion}, }, References: []*gen.SpanRef{ { @@ -535,8 +569,8 @@ func Test_spanSnapshotToThrift(t *testing.T) { StartTime: now.UnixNano() / 1000, Duration: 0, Tags: []*gen.Tag{ - {Key: "otel.library.name", VType: gen.TagType_STRING, VStr: &instrLibName}, - {Key: "otel.library.version", VType: gen.TagType_STRING, VStr: &instrLibVersion}, + {Key: keyInstrumentationLibraryName, VType: gen.TagType_STRING, VStr: &instrLibName}, + {Key: keyInstrumentationLibraryVersion, VType: gen.TagType_STRING, VStr: &instrLibVersion}, }, }, }, @@ -649,7 +683,7 @@ func TestJaegerBatchList(t *testing.T) { { OperationName: "s1", Tags: []*gen.Tag{ - {Key: "span.kind", VType: gen.TagType_STRING, VStr: &spanKind}, + {Key: keySpanKind, VType: gen.TagType_STRING, VStr: &spanKind}, }, StartTime: now.UnixNano() / 1000, }, From e0682240402a495511b2e9620f8d6299aa79164e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 31 Mar 2021 14:02:39 -0700 Subject: [PATCH 2/4] Add changes to changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eaf004e791..b6c8c8f46e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - The `Span.IsRecording` implementation from `go.opentelemetry.io/otel/sdk/trace` always returns false when not being sampled. (#1750) +- The jaeger exporter now correctly sets tags for the Span status code and message. + This means it uses the correct tag keys (`"otel.status_code"`, `"otel.status_description"`) and does not set the status message as a tag unless it is set on the span. (#1761) ### Changed From d6bc5eed27801008fb74f5adb77ed24a3f1bdf36 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Apr 2021 16:07:08 +0000 Subject: [PATCH 3/4] Update CHANGELOG.md Co-authored-by: Anthony Mirabella --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6c8c8f46e4..37ef8011c69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - The `Span.IsRecording` implementation from `go.opentelemetry.io/otel/sdk/trace` always returns false when not being sampled. (#1750) -- The jaeger exporter now correctly sets tags for the Span status code and message. +- The Jaeger exporter now correctly sets tags for the Span status code and message. This means it uses the correct tag keys (`"otel.status_code"`, `"otel.status_description"`) and does not set the status message as a tag unless it is set on the span. (#1761) ### Changed From 3ca96c603894e45bb9511d284c51e3947e0443e7 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Apr 2021 09:19:01 -0700 Subject: [PATCH 4/4] Fix misspell --- exporters/trace/jaeger/jaeger_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index b7ad912d0f8..4c5911bf825 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -392,7 +392,7 @@ func Test_spanSnapshotToThrift(t *testing.T) { {Key: keyInstrumentationLibraryName, VType: gen.TagType_STRING, VStr: &instrLibName}, {Key: keyInstrumentationLibraryVersion, VType: gen.TagType_STRING, VStr: &instrLibVersion}, {Key: keyStatusCode, VType: gen.TagType_LONG, VLong: &statusCodeValue}, - // Should not have a status message becuase it was unset + // Should not have a status message because it was unset {Key: keySpanKind, VType: gen.TagType_STRING, VStr: &spanKind}, }, },