Add Span#RecordError method to simplify adding error events to spans (#473)

* Add `Span#Error` method to simplify setting an error status and message.

* `Span#Error` should no-op on nil errors

* Record errors as a span event rather than status/attributes.

The implementation in the SDK package now relies on existing API methods.

* Add WithErrorStatus() ErrorOption to allow setting span status on error.

* Apply suggestions from code review

Co-Authored-By: Krzesimir Nowak <qdlacz@gmail.com>

* Address code review feedback

* Clean up RecordError tests

* Ensure complete and unique error type is recorded for defined types
* Avoid duplicating logic under test in tests

* Move TestError to internal/testing package, improve RecordError test scenarios

Co-authored-by: Krzesimir Nowak <qdlacz@gmail.com>

Author: Aneurysm9

api/trace/api.go

@@ -60,6 +60,29 @@ func WithEndTime(t time.Time) EndOption {
 	}
 }
 
+// ErrorConfig provides options to set properties of an error event at the time it is recorded.
+type ErrorConfig struct {
+	Timestamp time.Time
+	Status    codes.Code
+}
+
+// ErrorOption applies changes to ErrorConfig that sets options when an error event is recorded.
+type ErrorOption func(*ErrorConfig)
+
+// WithErrorTime sets the time at which the error event should be recorded.
+func WithErrorTime(t time.Time) ErrorOption {
+	return func(c *ErrorConfig) {
+		c.Timestamp = t
+	}
+}
+
+// WithErrorStatus indicates the span status that should be set when recording an error event.
+func WithErrorStatus(s codes.Code) ErrorOption {
+	return func(c *ErrorConfig) {
+		c.Status = s
+	}
+}
+
 type Span interface {
 	// Tracer returns tracer used to create this span. Tracer cannot be nil.
 	Tracer() Tracer
@@ -77,6 +100,9 @@ type Span interface {
 	// IsRecording returns true if the span is active and recording events is enabled.
 	IsRecording() bool
 
+	// RecordError records an error as a span event.
+	RecordError(ctx context.Context, err error, opts ...ErrorOption)
+
 	// SpanContext returns span context of the span. Returned SpanContext is usable
 	// even after the span ends.
 	SpanContext() core.SpanContext

api/trace/context_test.go

@@ -87,6 +87,10 @@ func (mockSpan) SetAttributes(attributes ...core.KeyValue) {
 func (mockSpan) End(options ...trace.EndOption) {
 }
 
+// RecordError does nothing.
+func (mockSpan) RecordError(ctx context.Context, err error, opts ...trace.ErrorOption) {
+}
+
 // Tracer returns noop implementation of Tracer.
 func (mockSpan) Tracer() trace.Tracer {
 	return trace.NoopTracer{}

api/trace/noop_span.go

@@ -54,6 +54,10 @@ func (NoopSpan) SetAttributes(attributes ...core.KeyValue) {
 func (NoopSpan) End(options ...EndOption) {
 }
 
+// RecordError does nothing.
+func (NoopSpan) RecordError(ctx context.Context, err error, opts ...ErrorOption) {
+}
+
 // Tracer returns noop implementation of Tracer.
 func (NoopSpan) Tracer() Tracer {
 	return NoopTracer{}

api/trace/testtrace/span.go

@@ -16,6 +16,8 @@ package testtrace
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 	"sync"
 	"time"
 
@@ -25,6 +27,12 @@ import (
 	"go.opentelemetry.io/otel/api/trace"
 )
 
+const (
+	errorTypeKey    = core.Key("error.type")
+	errorMessageKey = core.Key("error.message")
+	errorEventName  = "error"
+)
+
 var _ trace.Span = (*Span)(nil)
 
 type Span struct {
@@ -69,6 +77,36 @@ func (s *Span) End(opts ...trace.EndOption) {
 	s.ended = true
 }
 
+func (s *Span) RecordError(ctx context.Context, err error, opts ...trace.ErrorOption) {
+	if err == nil || s.ended {
+		return
+	}
+
+	cfg := trace.ErrorConfig{}
+	for _, o := range opts {
+		o(&cfg)
+	}
+
+	if cfg.Timestamp.IsZero() {
+		cfg.Timestamp = time.Now()
+	}
+
+	if cfg.Status != codes.OK {
+		s.SetStatus(cfg.Status)
+	}
+
+	errType := reflect.TypeOf(err)
+	errTypeString := fmt.Sprintf("%s.%s", errType.PkgPath(), errType.Name())
+	if errTypeString == "." {
+		errTypeString = errType.String()
+	}
+
+	s.AddEventWithTimestamp(ctx, cfg.Timestamp, errorEventName,
+		errorTypeKey.String(errTypeString),
+		errorMessageKey.String(err.Error()),
+	)
+}
+
 func (s *Span) AddEvent(ctx context.Context, name string, attrs ...core.KeyValue) {
 	s.AddEventWithTimestamp(ctx, time.Now(), name, attrs...)
 }

api/trace/testtrace/span_test.go

@@ -16,6 +16,7 @@ package testtrace_test
 
 import (
 	"context"
+	"errors"
 	"sync"
 	"testing"
 	"time"
@@ -26,6 +27,7 @@ import (
 	"go.opentelemetry.io/otel/api/trace"
 	"go.opentelemetry.io/otel/api/trace/testtrace"
 	"go.opentelemetry.io/otel/internal/matchers"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 )
 
 func TestSpan(t *testing.T) {
@@ -120,6 +122,115 @@ func TestSpan(t *testing.T) {
 		})
 	})
 
+	t.Run("#RecordError", func(t *testing.T) {
+		t.Run("records an error", func(t *testing.T) {
+			t.Parallel()
+
+			scenarios := []struct {
+				err error
+				typ string
+				msg string
+			}{
+				{
+					err: ottest.NewTestError("test error"),
+					typ: "go.opentelemetry.io/otel/internal/testing.TestError",
+					msg: "test error",
+				},
+				{
+					err: errors.New("test error 2"),
+					typ: "*errors.errorString",
+					msg: "test error 2",
+				},
+			}
+
+			for _, s := range scenarios {
+				e := matchers.NewExpecter(t)
+
+				tracer := testtrace.NewTracer()
+				ctx, span := tracer.Start(context.Background(), "test")
+
+				subject, ok := span.(*testtrace.Span)
+				e.Expect(ok).ToBeTrue()
+
+				testTime := time.Now()
+				subject.RecordError(ctx, s.err, trace.WithErrorTime(testTime))
+
+				expectedEvents := []testtrace.Event{{
+					Timestamp: testTime,
+					Name:      "error",
+					Attributes: map[core.Key]core.Value{
+						core.Key("error.type"):    core.String(s.typ),
+						core.Key("error.message"): core.String(s.msg),
+					},
+				}}
+				e.Expect(subject.Events()).ToEqual(expectedEvents)
+				e.Expect(subject.Status()).ToEqual(codes.OK)
+			}
+		})
+
+		t.Run("sets span status if provided", func(t *testing.T) {
+			t.Parallel()
+
+			e := matchers.NewExpecter(t)
+
+			tracer := testtrace.NewTracer()
+			ctx, span := tracer.Start(context.Background(), "test")
+
+			subject, ok := span.(*testtrace.Span)
+			e.Expect(ok).ToBeTrue()
+
+			errMsg := "test error message"
+			testErr := ottest.NewTestError(errMsg)
+			testTime := time.Now()
+			expStatus := codes.Unknown
+			subject.RecordError(ctx, testErr, trace.WithErrorTime(testTime), trace.WithErrorStatus(expStatus))
+
+			expectedEvents := []testtrace.Event{{
+				Timestamp: testTime,
+				Name:      "error",
+				Attributes: map[core.Key]core.Value{
+					core.Key("error.type"):    core.String("go.opentelemetry.io/otel/internal/testing.TestError"),
+					core.Key("error.message"): core.String(errMsg),
+				},
+			}}
+			e.Expect(subject.Events()).ToEqual(expectedEvents)
+			e.Expect(subject.Status()).ToEqual(expStatus)
+		})
+
+		t.Run("cannot be set after the span has ended", func(t *testing.T) {
+			t.Parallel()
+
+			e := matchers.NewExpecter(t)
+
+			tracer := testtrace.NewTracer()
+			ctx, span := tracer.Start(context.Background(), "test")
+
+			subject, ok := span.(*testtrace.Span)
+			e.Expect(ok).ToBeTrue()
+
+			subject.End()
+			subject.RecordError(ctx, errors.New("ignored error"))
+
+			e.Expect(len(subject.Events())).ToEqual(0)
+		})
+
+		t.Run("has no effect with nil error", func(t *testing.T) {
+			t.Parallel()
+
+			e := matchers.NewExpecter(t)
+
+			tracer := testtrace.NewTracer()
+			ctx, span := tracer.Start(context.Background(), "test")
+
+			subject, ok := span.(*testtrace.Span)
+			e.Expect(ok).ToBeTrue()
+
+			subject.RecordError(ctx, nil)
+
+			e.Expect(len(subject.Events())).ToEqual(0)
+		})
+	})
+
 	t.Run("#IsRecording", func(t *testing.T) {
 		t.Run("returns true", func(t *testing.T) {
 			t.Parallel()

bridge/opentracing/internal/mock.go

@@ -17,6 +17,7 @@ package internal
 import (
 	"context"
 	"math/rand"
+	"reflect"
 	"sync"
 	"time"
 
@@ -259,6 +260,35 @@ func (s *MockSpan) End(options ...oteltrace.EndOption) {
 	s.mockTracer.FinishedSpans = append(s.mockTracer.FinishedSpans, s)
 }
 
+func (s *MockSpan) RecordError(ctx context.Context, err error, opts ...oteltrace.ErrorOption) {
+	if err == nil {
+		return // no-op on nil error
+	}
+
+	if !s.EndTime.IsZero() {
+		return // already finished
+	}
+
+	cfg := oteltrace.ErrorConfig{}
+
+	for _, o := range opts {
+		o(&cfg)
+	}
+
+	if cfg.Timestamp.IsZero() {
+		cfg.Timestamp = time.Now()
+	}
+
+	if cfg.Status != codes.OK {
+		s.SetStatus(cfg.Status)
+	}
+
+	s.AddEventWithTimestamp(ctx, cfg.Timestamp, "error",
+		otelcore.Key("error.type").String(reflect.TypeOf(err).String()),
+		otelcore.Key("error.message").String(err.Error()),
+	)
+}
+
 func (s *MockSpan) Tracer() oteltrace.Tracer {
 	return s.officialTracer
 }

internal/testing/errors.go

@@ -0,0 +1,13 @@
+package testing
+
+type TestError string
+
+var _ error = TestError("")
+
+func NewTestError(s string) error {
+	return TestError(s)
+}
+
+func (e TestError) Error() string {
+	return string(e)
+}

internal/trace/mock_span.go

@@ -62,6 +62,10 @@ func (ms *MockSpan) SetAttributes(attributes ...core.KeyValue) {
 func (ms *MockSpan) End(options ...apitrace.EndOption) {
 }
 
+// RecordError does nothing.
+func (ms *MockSpan) RecordError(ctx context.Context, err error, opts ...apitrace.ErrorOption) {
+}
+
 // SetName does nothing.
 func (ms *MockSpan) SetName(name string) {
 }

sdk/trace/span.go

@@ -16,6 +16,8 @@ package trace
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 	"sync"
 	"time"
 
@@ -27,6 +29,12 @@ import (
 	"go.opentelemetry.io/otel/sdk/internal"
 )
 
+const (
+	errorTypeKey    = core.Key("error.type")
+	errorMessageKey = core.Key("error.message")
+	errorEventName  = "error"
+)
+
 // span implements apitrace.Span interface.
 type span struct {
 	// data contains information recorded about the span.
@@ -123,6 +131,42 @@ func (s *span) End(options ...apitrace.EndOption) {
 	})
 }
 
+func (s *span) RecordError(ctx context.Context, err error, opts ...apitrace.ErrorOption) {
+	if s == nil || err == nil {
+		return
+	}
+
+	if !s.IsRecording() {
+		return
+	}
+
+	cfg := apitrace.ErrorConfig{}
+
+	for _, o := range opts {
+		o(&cfg)
+	}
+
+	if cfg.Timestamp.IsZero() {
+		cfg.Timestamp = time.Now()
+	}
+
+	if cfg.Status != codes.OK {
+		s.SetStatus(cfg.Status)
+	}
+
+	errType := reflect.TypeOf(err)
+	errTypeString := fmt.Sprintf("%s.%s", errType.PkgPath(), errType.Name())
+	if errTypeString == "." {
+		// PkgPath() and Name() may be empty for builtin Types
+		errTypeString = errType.String()
+	}
+
+	s.AddEventWithTimestamp(ctx, cfg.Timestamp, errorEventName,
+		errorTypeKey.String(errTypeString),
+		errorMessageKey.String(err.Error()),
+	)
+}
+
 func (s *span) Tracer() apitrace.Tracer {
 	return s.tracer
 }