Improve trace status handling (#3214)

* Implement specification compliant trace status handling

* Update trace/trace.go

Co-authored-by: Damien Mathieu <42@dmathieu.com>

* Update CHANGELOG.md

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

* chore: Make linter happy

Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

CHANGELOG.md

@@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Changed
+
+- `span.SetStatus` has been updated such that calls that lower the status are now no-ops. (#3214)
+
 ## [0.32.1] Metric SDK (Alpha) - 2022-09-22
 
 ### Changed

sdk/trace/span.go

@@ -189,15 +189,18 @@ func (s *recordingSpan) SetStatus(code codes.Code, description string) {
 	if !s.IsRecording() {
 		return
 	}
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if s.status.Code > code {
+		return
+	}
 
 	status := Status{Code: code}
 	if code == codes.Error {
 		status.Description = description
 	}
 
-	s.mu.Lock()
 	s.status = status
-	s.mu.Unlock()
 }
 
 // SetAttributes sets attributes of this span.

sdk/trace/span_test.go

@@ -22,8 +22,84 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/codes"
 )
 
+func TestSetStatus(t *testing.T) {
+	tests := []struct {
+		name        string
+		span        recordingSpan
+		code        codes.Code
+		description string
+		expected    Status
+	}{
+		{
+			"Error and description should overwrite Unset",
+			recordingSpan{},
+			codes.Error,
+			"description",
+			Status{Code: codes.Error, Description: "description"},
+		},
+		{
+			"Ok should overwrite Unset and ignore description",
+			recordingSpan{},
+			codes.Ok,
+			"description",
+			Status{Code: codes.Ok},
+		},
+		{
+			"Error and description should return error and overwrite description",
+			recordingSpan{status: Status{Code: codes.Error, Description: "d1"}},
+			codes.Error,
+			"d2",
+			Status{Code: codes.Error, Description: "d2"},
+		},
+		{
+			"Ok should overwrite error and remove description",
+			recordingSpan{status: Status{Code: codes.Error, Description: "d1"}},
+			codes.Ok,
+			"d2",
+			Status{Code: codes.Ok},
+		},
+		{
+			"Error and description should be ignored when already Ok",
+			recordingSpan{status: Status{Code: codes.Ok}},
+			codes.Error,
+			"d2",
+			Status{Code: codes.Ok},
+		},
+		{
+			"Ok should be noop when already Ok",
+			recordingSpan{status: Status{Code: codes.Ok}},
+			codes.Ok,
+			"d2",
+			Status{Code: codes.Ok},
+		},
+		{
+			"Unset should be noop when already Ok",
+			recordingSpan{status: Status{Code: codes.Ok}},
+			codes.Unset,
+			"d2",
+			Status{Code: codes.Ok},
+		},
+		{
+			"Unset should be noop when already Error",
+			recordingSpan{status: Status{Code: codes.Error, Description: "d1"}},
+			codes.Unset,
+			"d2",
+			Status{Code: codes.Error, Description: "d1"},
+		},
+	}
+
+	for i := range tests {
+		tc := &tests[i]
+		t.Run(tc.name, func(t *testing.T) {
+			tc.span.SetStatus(tc.code, tc.description)
+			assert.Equal(t, tc.expected, tc.span.status)
+		})
+	}
+}
+
 func TestTruncateAttr(t *testing.T) {
 	const key = "key"
 

trace/trace.go

@@ -364,8 +364,9 @@ type Span interface {
 	SpanContext() SpanContext
 
 	// SetStatus sets the status of the Span in the form of a code and a
-	// description, overriding previous values set. The description is only
+	// description, provided the status hasn't already been set to a higher
-	// included in a status when the code is for an error.
+	// value before (OK > Error > Unset). The description is only included in a
+	// status when the code is for an error.
 	SetStatus(code codes.Code, description string)
 
 	// SetName sets the Span name.