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.

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

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

Merged
merged 10 commits into from
Feb 28, 2020
26 changes: 26 additions & 0 deletions api/trace/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions api/trace/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
4 changes: 4 additions & 0 deletions api/trace/noop_span.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
38 changes: 38 additions & 0 deletions api/trace/testtrace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package testtrace

import (
"context"
"fmt"
"reflect"
"sync"
"time"

Expand All @@ -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 {
Expand Down Expand Up @@ -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{}
Aneurysm9 marked this conversation as resolved.
Show resolved Hide resolved
for _, o := range opts {
o(&cfg)
}

if cfg.Timestamp.IsZero() {
cfg.Timestamp = time.Now()
}

if cfg.Status != codes.OK {
s.SetStatus(cfg.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line raises questions for me about the SetStatus API, which is tangential to this PR. The Span.Status field has two fields, the code (passed here) and a message. This should probably set the same err.Error() as used for the event message, but the SetStatus API doesn't accept a message string (which is a separate matter).

This has me wondering whether we want to use RecordError(..., WithStatusCode()) as the conventional pattern for recording an error and setting the status. I'm not sure I'd pick this approach -- I'm inclined for there to be two methods, one to record an error event, and one to record an error event plus set the status (i.e., two methods RecordErrorEvent and RecordErrorStatus).

}

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...)
}
Expand Down
111 changes: 111 additions & 0 deletions api/trace/testtrace/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package testtrace_test

import (
"context"
"errors"
"sync"
"testing"
"time"
Expand All @@ -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) {
Expand Down Expand Up @@ -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()
Expand Down
30 changes: 30 additions & 0 deletions bridge/opentracing/internal/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package internal
import (
"context"
"math/rand"
"reflect"
"sync"
"time"

Expand Down Expand Up @@ -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
}
Expand Down
13 changes: 13 additions & 0 deletions internal/testing/errors.go
Original file line number Diff line number Diff line change
@@ -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)
}
4 changes: 4 additions & 0 deletions internal/trace/mock_span.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
}
Expand Down
44 changes: 44 additions & 0 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package trace

import (
"context"
"fmt"
"reflect"
"sync"
"time"

Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down
Loading