Skip to content

Commit

Permalink
Remove accessors for deprecated status code, fix receiver logic (#2521)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* Add changelog entry

Signed-off-by: Bogdan Drutu <[email protected]>
  • Loading branch information
@bogdandrutu
bogdandrutu authored Feb 22, 2021
1 parent 9383e82 commit a04feee
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 145 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
## 🛑 Breaking changes 🛑

- Remove deprecated function `IsValid` from trace/span ID (#2522)
- Remove accessors for deprecated status code (#2521)

## 🧰 Bug fixes 🧰

- `otlp` receiver: Sets the correct deprecated status code before sending data to the pipeline (#2521)

## v0.20.0 Beta

Expand Down
8 changes: 0 additions & 8 deletions cmd/pdatagen/internal/trace_structs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 0 additions & 11 deletions consumer/pdata/generated_trace.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 0 additions & 9 deletions consumer/pdata/generated_trace_test.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 4 additions & 42 deletions consumer/pdata/trace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,35 +121,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
Expand All @@ -166,21 +137,12 @@ func (sc StatusCode) String() string { return otlptrace.Status_StatusCode(sc).St
func (ms SpanStatus) SetCode(v StatusCode) {
ms.orig.Code = otlptrace.Status_StatusCode(v)

// According to OTLP spec we also need to set the deprecated_code field.
// See https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L231
//
// if code==STATUS_CODE_UNSET then `deprecated_code` MUST be
// set to DEPRECATED_STATUS_CODE_OK.
//
// if code==STATUS_CODE_OK then `deprecated_code` MUST be
// set to DEPRECATED_STATUS_CODE_OK.
//
// if code==STATUS_CODE_ERROR then `deprecated_code` MUST be
// set to DEPRECATED_STATUS_CODE_UNKNOWN_ERROR.
// According to OTLP spec we also need to set the deprecated_code field as we are a new sender:
// See https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L239
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
}
}
15 changes: 6 additions & 9 deletions consumer/pdata/trace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
30 changes: 13 additions & 17 deletions receiver/otlpreceiver/trace/otlp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,22 @@ func (r *Receiver) Export(ctx context.Context, req *collectortrace.ExportTraceSe
ctxWithReceiverName := obsreport.ReceiverContext(ctx, r.instanceName, receiverTransport)

// Perform backward compatibility conversion of Span Status code according to
// OTLP specification.
// See https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L231
//
// If code==STATUS_CODE_UNSET then the value of `deprecated_code` is the
// carrier of the overall status according to these rules:
//
// if deprecated_code==DEPRECATED_STATUS_CODE_OK then the receiver MUST interpret
// the overall status to be STATUS_CODE_UNSET.
//
// if deprecated_code!=DEPRECATED_STATUS_CODE_OK then the receiver MUST interpret
// the overall status to be STATUS_CODE_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.
// OTLP specification as we are a new receiver and sender (we are pushing data to the pipelines):
// See https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L239
// See https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L253
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
}
}
}
Expand Down
109 changes: 60 additions & 49 deletions receiver/otlpreceiver/trace/otlp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,93 +196,104 @@ 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
// carrier of the overall status according to these rules:
//
// 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)
})
}
}

0 comments on commit a04feee

Please sign in to comment.