Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove accessors for deprecated status code, fix receiver logic #2521

Merged
merged 2 commits into from
Feb 22, 2021
Merged

Remove accessors for deprecated status code, fix receiver logic #2521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ceda5aa
Remove accessors for deprecated status code, fix receiver logic
bogdandrutu Feb 20, 2021
c51d6a1
Add changelog entry
bogdandrutu Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop

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
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Comment on lines +69 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these changes? The spec says that receivers MUST:

// If code!=STATUS_CODE_UNSET then the value of deprecated_code MUST be
// ignored, the code field is the sole carrier of the status.

This does not seem to match the spec anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the spec says? Why does "SetCode" updates deprecated but in this case we don't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comments to SetCode as well as for the receiver to clarify when we are sender vs receiver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline, so yes, OTLP receiver is both a "receiver" and "sender" in the sense that the proto uses these words here: https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L239

}
}
}
Expand Down
109 changes: 60 additions & 49 deletions receiver/otlpreceiver/trace/otlp_test.go
View file
Open in desktop
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)
})
}
}