From 52164acf56b466eee2f940d91349e5733a923bc1 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Sat, 20 Feb 2021 13:47:43 -0800 Subject: [PATCH] Remove accessors for deprecated status code, fix receiver logic The previous logic was to ignore deprecated if received non unset for new status code, but if downstream a service is not upgraded it should see the deprecated status set correctly. This change is to be consistent with `SetCode`. Signed-off-by: Bogdan Drutu --- cmd/pdatagen/internal/trace_structs.go | 8 -- consumer/pdata/generated_trace.go | 15 ---- consumer/pdata/generated_trace_test.go | 9 -- consumer/pdata/trace.go | 33 +------ consumer/pdata/trace_test.go | 15 ++-- receiver/otlpreceiver/trace/otlp.go | 13 ++- receiver/otlpreceiver/trace/otlp_test.go | 109 +++++++++++++---------- 7 files changed, 78 insertions(+), 124 deletions(-) diff --git a/cmd/pdatagen/internal/trace_structs.go b/cmd/pdatagen/internal/trace_structs.go index fd42c8b080c..3d6c5d23838 100644 --- a/cmd/pdatagen/internal/trace_structs.go +++ b/cmd/pdatagen/internal/trace_structs.go @@ -196,14 +196,6 @@ var spanStatus = &messageValueStruct{ // to OTLP spec https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L231 manualSetter: true, }, - &primitiveTypedField{ - fieldName: "DeprecatedCode", - originFieldName: "DeprecatedCode", - returnType: "DeprecatedStatusCode", - rawType: "otlptrace.Status_DeprecatedStatusCode", - defaultVal: "DeprecatedStatusCode(0)", - testVal: "DeprecatedStatusCode(1)", - }, &primitiveField{ fieldName: "Message", originFieldName: "Message", diff --git a/consumer/pdata/generated_trace.go b/consumer/pdata/generated_trace.go index cec4af87a70..8d8bb85d56e 100644 --- a/consumer/pdata/generated_trace.go +++ b/consumer/pdata/generated_trace.go @@ -1105,20 +1105,6 @@ func (ms SpanStatus) Code() StatusCode { return StatusCode((*ms.orig).Code) } -// DeprecatedCode returns the deprecatedcode associated with this SpanStatus. -// -// Important: This causes a runtime error if IsNil() returns "true". -func (ms SpanStatus) DeprecatedCode() DeprecatedStatusCode { - return DeprecatedStatusCode((*ms.orig).DeprecatedCode) -} - -// SetDeprecatedCode replaces the deprecatedcode associated with this SpanStatus. -// -// Important: This causes a runtime error if IsNil() returns "true". -func (ms SpanStatus) SetDeprecatedCode(v DeprecatedStatusCode) { - (*ms.orig).DeprecatedCode = otlptrace.Status_DeprecatedStatusCode(v) -} - // Message returns the message associated with this SpanStatus. // // Important: This causes a runtime error if IsNil() returns "true". @@ -1136,6 +1122,5 @@ func (ms SpanStatus) SetMessage(v string) { // CopyTo copies all properties from the current struct to the dest. func (ms SpanStatus) CopyTo(dest SpanStatus) { dest.SetCode(ms.Code()) - dest.SetDeprecatedCode(ms.DeprecatedCode()) dest.SetMessage(ms.Message()) } diff --git a/consumer/pdata/generated_trace_test.go b/consumer/pdata/generated_trace_test.go index 950e21fd85c..70439b40952 100644 --- a/consumer/pdata/generated_trace_test.go +++ b/consumer/pdata/generated_trace_test.go @@ -857,14 +857,6 @@ func TestSpanStatus_Code(t *testing.T) { assert.EqualValues(t, testValCode, ms.Code()) } -func TestSpanStatus_DeprecatedCode(t *testing.T) { - ms := NewSpanStatus() - assert.EqualValues(t, DeprecatedStatusCode(0), ms.DeprecatedCode()) - testValDeprecatedCode := DeprecatedStatusCode(1) - ms.SetDeprecatedCode(testValDeprecatedCode) - assert.EqualValues(t, testValDeprecatedCode, ms.DeprecatedCode()) -} - func TestSpanStatus_Message(t *testing.T) { ms := NewSpanStatus() assert.EqualValues(t, "", ms.Message()) @@ -1019,6 +1011,5 @@ func generateTestSpanStatus() SpanStatus { func fillTestSpanStatus(tv SpanStatus) { tv.SetCode(StatusCode(1)) - tv.SetDeprecatedCode(DeprecatedStatusCode(1)) tv.SetMessage("cancelled") } diff --git a/consumer/pdata/trace.go b/consumer/pdata/trace.go index 4da548f987d..cbe7cea45fc 100644 --- a/consumer/pdata/trace.go +++ b/consumer/pdata/trace.go @@ -123,35 +123,6 @@ const ( SpanKindCONSUMER = SpanKind(otlptrace.Span_SPAN_KIND_CONSUMER) ) -// DeprecatedStatusCode is the deprecated status code used previously. -// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status -// Deprecated: use StatusCode instead. -type DeprecatedStatusCode otlptrace.Status_DeprecatedStatusCode - -const ( - DeprecatedStatusCodeOk = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_OK) - DeprecatedStatusCodeCancelled = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_CANCELLED) - DeprecatedStatusCodeUnknownError = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR) - DeprecatedStatusCodeInvalidArgument = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_INVALID_ARGUMENT) - DeprecatedStatusCodeDeadlineExceeded = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_DEADLINE_EXCEEDED) - DeprecatedStatusCodeNotFound = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_NOT_FOUND) - DeprecatedStatusCodeAlreadyExists = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_ALREADY_EXISTS) - DeprecatedStatusCodePermissionDenied = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_PERMISSION_DENIED) - DeprecatedStatusCodeResourceExhausted = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_RESOURCE_EXHAUSTED) - DeprecatedStatusCodeFailedPrecondition = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_FAILED_PRECONDITION) - DeprecatedStatusCodeAborted = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_ABORTED) - DeprecatedStatusCodeOutOfRange = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_OUT_OF_RANGE) - DeprecatedStatusCodeUnimplemented = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_UNIMPLEMENTED) - DeprecatedStatusCodeInternalError = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_INTERNAL_ERROR) - DeprecatedStatusCodeUnavailable = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_UNAVAILABLE) - DeprecatedStatusCodeDataLoss = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_DATA_LOSS) - DeprecatedStatusCodeUnauthenticated = DeprecatedStatusCode(otlptrace.Status_DEPRECATED_STATUS_CODE_UNAUTHENTICATED) -) - -func (sc DeprecatedStatusCode) String() string { - return otlptrace.Status_DeprecatedStatusCode(sc).String() -} - // StatusCode mirrors the codes defined at // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status type StatusCode otlptrace.Status_StatusCode @@ -181,8 +152,8 @@ func (ms SpanStatus) SetCode(v StatusCode) { // set to DEPRECATED_STATUS_CODE_UNKNOWN_ERROR. switch v { case StatusCodeUnset, StatusCodeOk: - ms.SetDeprecatedCode(DeprecatedStatusCodeOk) + ms.orig.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_OK case StatusCodeError: - ms.SetDeprecatedCode(DeprecatedStatusCodeUnknownError) + ms.orig.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR } } diff --git a/consumer/pdata/trace_test.go b/consumer/pdata/trace_test.go index efa8001e111..07afd4972e6 100644 --- a/consumer/pdata/trace_test.go +++ b/consumer/pdata/trace_test.go @@ -128,24 +128,21 @@ func TestSpanStatusCode(t *testing.T) { // // if code==STATUS_CODE_UNSET then `deprecated_code` MUST be // set to DEPRECATED_STATUS_CODE_OK. - status.SetDeprecatedCode(DeprecatedStatusCodeUnknownError) - assert.EqualValues(t, DeprecatedStatusCodeUnknownError, status.DeprecatedCode()) + status.orig.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR status.SetCode(StatusCodeUnset) - assert.EqualValues(t, DeprecatedStatusCodeOk, status.DeprecatedCode()) + assert.EqualValues(t, otlptrace.Status_DEPRECATED_STATUS_CODE_OK, status.orig.DeprecatedCode) // if code==STATUS_CODE_OK then `deprecated_code` MUST be // set to DEPRECATED_STATUS_CODE_OK. - status.SetDeprecatedCode(DeprecatedStatusCodeUnknownError) - assert.EqualValues(t, DeprecatedStatusCodeUnknownError, status.DeprecatedCode()) + status.orig.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR status.SetCode(StatusCodeOk) - assert.EqualValues(t, DeprecatedStatusCodeOk, status.DeprecatedCode()) + assert.EqualValues(t, otlptrace.Status_DEPRECATED_STATUS_CODE_OK, status.orig.DeprecatedCode) // if code==STATUS_CODE_ERROR then `deprecated_code` MUST be // set to DEPRECATED_STATUS_CODE_UNKNOWN_ERROR. - status.SetDeprecatedCode(DeprecatedStatusCodeOk) - assert.EqualValues(t, DeprecatedStatusCodeOk, status.DeprecatedCode()) + status.orig.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_OK status.SetCode(StatusCodeError) - assert.EqualValues(t, DeprecatedStatusCodeUnknownError, status.DeprecatedCode()) + assert.EqualValues(t, otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, status.orig.DeprecatedCode) } func TestToFromOtlp(t *testing.T) { diff --git a/receiver/otlpreceiver/trace/otlp.go b/receiver/otlpreceiver/trace/otlp.go index 851a8275b1d..8eeca08bbfd 100644 --- a/receiver/otlpreceiver/trace/otlp.go +++ b/receiver/otlpreceiver/trace/otlp.go @@ -72,9 +72,16 @@ func (r *Receiver) Export(ctx context.Context, req *collectortrace.ExportTraceSe for _, rss := range req.ResourceSpans { for _, ils := range rss.InstrumentationLibrarySpans { for _, span := range ils.Spans { - if span.Status.Code == otlptrace.Status_STATUS_CODE_UNSET && - span.Status.DeprecatedCode != otlptrace.Status_DEPRECATED_STATUS_CODE_OK { - span.Status.Code = otlptrace.Status_STATUS_CODE_ERROR + switch span.Status.Code { + case otlptrace.Status_STATUS_CODE_UNSET: + if span.Status.DeprecatedCode != otlptrace.Status_DEPRECATED_STATUS_CODE_OK { + span.Status.Code = otlptrace.Status_STATUS_CODE_ERROR + } + case otlptrace.Status_STATUS_CODE_OK: + // If status code is set then overwrites deprecated. + span.Status.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_OK + case otlptrace.Status_STATUS_CODE_ERROR: + span.Status.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR } } } diff --git a/receiver/otlpreceiver/trace/otlp_test.go b/receiver/otlpreceiver/trace/otlp_test.go index 350367a6aeb..657f990f723 100644 --- a/receiver/otlpreceiver/trace/otlp_test.go +++ b/receiver/otlpreceiver/trace/otlp_test.go @@ -196,9 +196,10 @@ func TestDeprecatedStatusCode(t *testing.T) { // See specification for handling status code here: // https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L231 tests := []struct { - sendCode otlptrace.Status_StatusCode - sendDeprecatedCode otlptrace.Status_DeprecatedStatusCode - expectedRcvCode otlptrace.Status_StatusCode + sendCode otlptrace.Status_StatusCode + sendDeprecatedCode otlptrace.Status_DeprecatedStatusCode + expectedRcvCode otlptrace.Status_StatusCode + expectedDeprecatedCode otlptrace.Status_DeprecatedStatusCode }{ { // If code==STATUS_CODE_UNSET then the value of `deprecated_code` is the @@ -206,83 +207,93 @@ func TestDeprecatedStatusCode(t *testing.T) { // // if deprecated_code==DEPRECATED_STATUS_CODE_OK then the receiver MUST interpret // the overall status to be STATUS_CODE_UNSET. - sendCode: otlptrace.Status_STATUS_CODE_UNSET, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, - expectedRcvCode: otlptrace.Status_STATUS_CODE_UNSET, + sendCode: otlptrace.Status_STATUS_CODE_UNSET, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, + expectedRcvCode: otlptrace.Status_STATUS_CODE_UNSET, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, }, { // if deprecated_code!=DEPRECATED_STATUS_CODE_OK then the receiver MUST interpret // the overall status to be STATUS_CODE_ERROR. - sendCode: otlptrace.Status_STATUS_CODE_UNSET, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, - expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + sendCode: otlptrace.Status_STATUS_CODE_UNSET, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_ABORTED, + expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_ABORTED, }, { // If code!=STATUS_CODE_UNSET then the value of `deprecated_code` MUST be - // ignored, the `code` field is the sole carrier of the status. - sendCode: otlptrace.Status_STATUS_CODE_OK, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, - expectedRcvCode: otlptrace.Status_STATUS_CODE_OK, + // overwritten, the `code` field is the sole carrier of the status. + sendCode: otlptrace.Status_STATUS_CODE_OK, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, + expectedRcvCode: otlptrace.Status_STATUS_CODE_OK, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, }, { // If code!=STATUS_CODE_UNSET then the value of `deprecated_code` MUST be - // ignored, the `code` field is the sole carrier of the status. - sendCode: otlptrace.Status_STATUS_CODE_OK, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, - expectedRcvCode: otlptrace.Status_STATUS_CODE_OK, + // overwritten, the `code` field is the sole carrier of the status. + sendCode: otlptrace.Status_STATUS_CODE_OK, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, + expectedRcvCode: otlptrace.Status_STATUS_CODE_OK, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, }, { // If code!=STATUS_CODE_UNSET then the value of `deprecated_code` MUST be - // ignored, the `code` field is the sole carrier of the status. - sendCode: otlptrace.Status_STATUS_CODE_ERROR, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, - expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + // overwritten, the `code` field is the sole carrier of the status. + sendCode: otlptrace.Status_STATUS_CODE_ERROR, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_OK, + expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, }, { // If code!=STATUS_CODE_UNSET then the value of `deprecated_code` MUST be - // ignored, the `code` field is the sole carrier of the status. - sendCode: otlptrace.Status_STATUS_CODE_ERROR, - sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, - expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + // overwritten, the `code` field is the sole carrier of the status. + sendCode: otlptrace.Status_STATUS_CODE_ERROR, + sendDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, + expectedRcvCode: otlptrace.Status_STATUS_CODE_ERROR, + expectedDeprecatedCode: otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, }, } for _, test := range tests { - resourceSpans := []*otlptrace.ResourceSpans{ - { - InstrumentationLibrarySpans: []*otlptrace.InstrumentationLibrarySpans{ - { - Spans: []*otlptrace.Span{ - { - Status: otlptrace.Status{ - Code: test.sendCode, - DeprecatedCode: test.sendDeprecatedCode, + t.Run(test.sendCode.String()+"/"+test.sendDeprecatedCode.String(), func(t *testing.T) { + resourceSpans := []*otlptrace.ResourceSpans{ + { + InstrumentationLibrarySpans: []*otlptrace.InstrumentationLibrarySpans{ + { + Spans: []*otlptrace.Span{ + { + Status: otlptrace.Status{ + Code: test.sendCode, + DeprecatedCode: test.sendDeprecatedCode, + }, }, }, }, }, }, - }, - } + } - req := &collectortrace.ExportTraceServiceRequest{ - ResourceSpans: resourceSpans, - } + req := &collectortrace.ExportTraceServiceRequest{ + ResourceSpans: resourceSpans, + } + + traceSink.Reset() - traceSink.Reset() + resp, err := traceClient.Export(context.Background(), req) + require.NoError(t, err, "Failed to export trace: %v", err) + require.NotNil(t, resp, "The response is missing") - resp, err := traceClient.Export(context.Background(), req) - require.NoError(t, err, "Failed to export trace: %v", err) - require.NotNil(t, resp, "The response is missing") + require.Equal(t, 1, len(traceSink.AllTraces()), "unexpected length: %v", len(traceSink.AllTraces())) - require.Equal(t, 1, len(traceSink.AllTraces()), "unexpected length: %v", len(traceSink.AllTraces())) + rcvdStatus := traceSink.AllTraces()[0].ResourceSpans().At(0).InstrumentationLibrarySpans().At(0).Spans().At(0).Status() - rcvdStatus := traceSink.AllTraces()[0].ResourceSpans().At(0).InstrumentationLibrarySpans().At(0).Spans().At(0).Status() + // Check that Code is as expected. + assert.EqualValues(t, rcvdStatus.Code(), test.expectedRcvCode) - // Check that Code is as expected. - assert.EqualValues(t, rcvdStatus.Code(), test.expectedRcvCode) + spanProto := pdata.TracesToOtlp(traceSink.AllTraces()[0])[0].InstrumentationLibrarySpans[0].Spans[0] - // Check that DeprecatedCode is passed as is. - assert.EqualValues(t, rcvdStatus.DeprecatedCode(), test.sendDeprecatedCode) + // Check that DeprecatedCode is passed as is. + assert.EqualValues(t, spanProto.Status.DeprecatedCode, test.expectedDeprecatedCode) + }) } }