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 WithStatus option for use with RecordError #2887

Closed
Closed
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Added

- Add a `WithStatus` option to `go.opentelemetry.io/otel/trace` to allow setting the span status when calling `span.RecordError`. (#1677)

## [1.7.0/0.30.0] - 2022-04-28

### Added
16 changes: 13 additions & 3 deletions bridge/opentracing/internal/mock.go
Original file line number Diff line number Diff line change
@@ -256,7 +256,7 @@ func (s *MockSpan) End(options ...trace.SpanEndOption) {
s.mockTracer.FinishedSpans = append(s.mockTracer.FinishedSpans, s)
}

func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
func (s *MockSpan) RecordError(err error, opts ...trace.ErrorOption) {
if err == nil {
return // no-op on nil error
}
@@ -265,12 +265,22 @@ func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
return // already finished
}

s.SetStatus(codes.Error, "")
opts = append(opts, trace.WithAttributes(
semconv.ExceptionTypeKey.String(reflect.TypeOf(err).String()),
semconv.ExceptionMessageKey.String(err.Error()),
))
s.AddEvent(semconv.ExceptionEventName, opts...)

c := trace.NewErrorConfig(opts...)
if c.SetErrorStatus() {
s.SetStatus(codes.Error, err.Error())
}

var eventOpts []trace.EventOption
for _, opt := range opts {
eventOpts = append(eventOpts, opt)
}

s.AddEvent(semconv.ExceptionEventName, eventOpts...)
}

func (s *MockSpan) Tracer() trace.Tracer {
5 changes: 1 addition & 4 deletions internal/global/trace.go
Original file line number Diff line number Diff line change
@@ -171,17 +171,14 @@ func (nonRecordingSpan) IsRecording() bool { return false }
// SetStatus does nothing.
func (nonRecordingSpan) SetStatus(codes.Code, string) {}

// SetError does nothing.
func (nonRecordingSpan) SetError(bool) {}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this method is being removed? It may be a cleanup. But then, I think it'd be better to do the removal in its own PR.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry about that. I removed it as it looked like it was defunct, then decided I shouldn't be doing things like that in the same PR but didn't clean up all my clean-ups!


// SetAttributes does nothing.
func (nonRecordingSpan) SetAttributes(...attribute.KeyValue) {}

// End does nothing.
func (nonRecordingSpan) End(...trace.SpanEndOption) {}

// RecordError does nothing.
func (nonRecordingSpan) RecordError(error, ...trace.EventOption) {}
func (nonRecordingSpan) RecordError(error, ...trace.ErrorOption) {}

// AddEvent does nothing.
func (nonRecordingSpan) AddEvent(string, ...trace.EventOption) {}
25 changes: 16 additions & 9 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
@@ -401,7 +401,7 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) {
// SetStatus is required if the Status of the Span should be set to Error, this method
// does not change the Span status. If this span is not being recorded or err is nil
// than this method does nothing.
func (s *recordingSpan) RecordError(err error, opts ...trace.EventOption) {
func (s *recordingSpan) RecordError(err error, opts ...trace.ErrorOption) {
if s == nil || err == nil || !s.IsRecording() {
return
}
@@ -411,14 +411,24 @@ func (s *recordingSpan) RecordError(err error, opts ...trace.EventOption) {
semconv.ExceptionMessageKey.String(err.Error()),
))

c := trace.NewEventConfig(opts...)
if c.StackTrace() {
opts = append(opts, trace.WithAttributes(
c := trace.NewErrorConfig(opts...)
if c.SetErrorStatus() {
s.SetStatus(codes.Error, err.Error())
}

var eventOpts []trace.EventOption
for _, opt := range opts {
eventOpts = append(eventOpts, opt)
}

eventConfig := trace.NewEventConfig(eventOpts...)
if eventConfig.StackTrace() {
eventOpts = append(eventOpts, trace.WithAttributes(
semconv.ExceptionStacktraceKey.String(recordStackTrace()),
))
}

s.addEvent(semconv.ExceptionEventName, opts...)
s.addEvent(semconv.ExceptionEventName, eventOpts...)
}

func typeStr(i interface{}) string {
@@ -757,17 +767,14 @@ func (nonRecordingSpan) IsRecording() bool { return false }
// SetStatus does nothing.
func (nonRecordingSpan) SetStatus(codes.Code, string) {}

// SetError does nothing.
func (nonRecordingSpan) SetError(bool) {}

// SetAttributes does nothing.
func (nonRecordingSpan) SetAttributes(...attribute.KeyValue) {}

// End does nothing.
func (nonRecordingSpan) End(...trace.SpanEndOption) {}

// RecordError does nothing.
func (nonRecordingSpan) RecordError(error, ...trace.EventOption) {}
func (nonRecordingSpan) RecordError(error, ...trace.ErrorOption) {}

// AddEvent does nothing.
func (nonRecordingSpan) AddEvent(string, ...trace.EventOption) {}
26 changes: 22 additions & 4 deletions sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
@@ -1182,9 +1182,10 @@ func TestCustomStartEndTime(t *testing.T) {

func TestRecordError(t *testing.T) {
scenarios := []struct {
err error
typ string
msg string
err error
typ string
msg string
withStatus bool
}{
{
err: ottest.NewTestError("test error"),
@@ -1196,6 +1197,12 @@ func TestRecordError(t *testing.T) {
typ: "*errors.errorString",
msg: "test error 2",
},
{
err: errors.New("test error 3"),
typ: "*errors.errorString",
msg: "test error 3",
withStatus: true,
},
}

for _, s := range scenarios {
@@ -1204,7 +1211,12 @@ func TestRecordError(t *testing.T) {
span := startSpan(tp, "RecordError")

errTime := time.Now()
span.RecordError(s.err, trace.WithTimestamp(errTime))
opts := []trace.ErrorOption{trace.WithTimestamp(errTime)}
if s.withStatus {
opts = append(opts, trace.WithStatus())
}

span.RecordError(s.err, opts...)

got, err := endSpan(te, span)
if err != nil {
@@ -1232,6 +1244,12 @@ func TestRecordError(t *testing.T) {
},
instrumentationLibrary: instrumentation.Library{Name: "RecordError"},
}

if s.withStatus {
want.status.Code = codes.Error
want.status.Description = s.msg
}

if diff := cmpDiff(got, want); diff != "" {
t.Errorf("SpanErrorOptions: -got +want %s", diff)
}
68 changes: 57 additions & 11 deletions trace/config.go
Original file line number Diff line number Diff line change
@@ -189,16 +189,42 @@ type SpanOption interface {
SpanEndOption
}

// SpanStartEventOption are options that can be used at the start of a span, or with an event.
type SpanStartEventOption interface {
// SpanStartEventErrorOption are options that can be used at the start of a span, or with an event, or when recording
// an error.
type SpanStartEventErrorOption interface {
SpanStartOption
EventOption
ErrorOption
}

// SpanEndEventOption are options that can be used at the end of a span, or with an event.
type SpanEndEventOption interface {
// SpanEndEventErrorOption are options that can be used at the end of a span, or with an event, or when recording an
// error.
type SpanEndEventErrorOption interface {
SpanEndOption
EventOption
ErrorOption
}

type ErrorConfig struct {
setErrorStatus bool
}

func (cfg *ErrorConfig) SetErrorStatus() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Calling this SetErrorStatus kind of hints at a setter, which this isn't. How about naming the variable hasErrorStatus, and this method HasErrorStatus?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, will fix

return cfg.setErrorStatus
}

func NewErrorConfig(options ...ErrorOption) ErrorConfig {
var c ErrorConfig
for _, option := range options {
c = option.applyError(c)
}
return c
}

// ErrorOption are options that can be used when recording an error to a span.
type ErrorOption interface {
applyError(ErrorConfig) ErrorConfig
EventOption
}

type attributeOption []attribute.KeyValue
@@ -208,12 +234,13 @@ func (o attributeOption) applySpan(c SpanConfig) SpanConfig {
return c
}
func (o attributeOption) applySpanStart(c SpanConfig) SpanConfig { return o.applySpan(c) }
func (o attributeOption) applyError(c ErrorConfig) ErrorConfig { return c }
func (o attributeOption) applyEvent(c EventConfig) EventConfig {
c.attributes = append(c.attributes, []attribute.KeyValue(o)...)
return c
}

var _ SpanStartEventOption = attributeOption{}
var _ SpanStartEventErrorOption = attributeOption{}

// WithAttributes adds the attributes related to a span life-cycle event.
// These attributes are used to describe the work a Span represents when this
@@ -224,14 +251,15 @@ var _ SpanStartEventOption = attributeOption{}
// If multiple of these options are passed the attributes of each successive
// option will extend the attributes instead of overwriting. There is no
// guarantee of uniqueness in the resulting attributes.
func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption {
func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventErrorOption {
return attributeOption(attributes)
}

// SpanEventOption are options that can be used with an event or a span.
type SpanEventOption interface {
// SpanEventErrorOption are options that can be used with an event or a span, or when recording an error.
type SpanEventErrorOption interface {
SpanOption
EventOption
ErrorOption
}

type timestampOption time.Time
@@ -242,21 +270,25 @@ func (o timestampOption) applySpan(c SpanConfig) SpanConfig {
}
func (o timestampOption) applySpanStart(c SpanConfig) SpanConfig { return o.applySpan(c) }
func (o timestampOption) applySpanEnd(c SpanConfig) SpanConfig { return o.applySpan(c) }
func (o timestampOption) applyError(c ErrorConfig) ErrorConfig {
return c
}
func (o timestampOption) applyEvent(c EventConfig) EventConfig {
c.timestamp = time.Time(o)
return c
}

var _ SpanEventOption = timestampOption{}
var _ SpanEventErrorOption = timestampOption{}

// WithTimestamp sets the time of a Span or Event life-cycle moment (e.g.
// started, stopped, errored).
func WithTimestamp(t time.Time) SpanEventOption {
func WithTimestamp(t time.Time) SpanEventErrorOption {
return timestampOption(t)
}

type stackTraceOption bool

func (o stackTraceOption) applyError(c ErrorConfig) ErrorConfig { return c }
func (o stackTraceOption) applyEvent(c EventConfig) EventConfig {
c.stackTrace = bool(o)
return c
@@ -267,8 +299,22 @@ func (o stackTraceOption) applySpan(c SpanConfig) SpanConfig {
}
func (o stackTraceOption) applySpanEnd(c SpanConfig) SpanConfig { return o.applySpan(c) }

// WithStatus is used when recording an error, and sets the span status to "Error" and
// adds the error's Error string as the description.
func WithStatus() ErrorOption {
return spanErrorStatus{}
}

type spanErrorStatus struct{}

func (o spanErrorStatus) applyError(c ErrorConfig) ErrorConfig {
c.setErrorStatus = true
return c
}
func (o spanErrorStatus) applyEvent(c EventConfig) EventConfig { return c }

// WithStackTrace sets the flag to capture the error with stack trace (e.g. true, false).
func WithStackTrace(b bool) SpanEndEventOption {
func WithStackTrace(b bool) SpanEndEventErrorOption {
return stackTraceOption(b)
}

5 changes: 1 addition & 4 deletions trace/noop.go
Original file line number Diff line number Diff line change
@@ -67,17 +67,14 @@ func (noopSpan) IsRecording() bool { return false }
// SetStatus does nothing.
func (noopSpan) SetStatus(codes.Code, string) {}

// SetError does nothing.
func (noopSpan) SetError(bool) {}

// SetAttributes does nothing.
func (noopSpan) SetAttributes(...attribute.KeyValue) {}

// End does nothing.
func (noopSpan) End(...SpanEndOption) {}

// RecordError does nothing.
func (noopSpan) RecordError(error, ...EventOption) {}
func (noopSpan) RecordError(error, ...ErrorOption) {}

// AddEvent does nothing.
func (noopSpan) AddEvent(string, ...EventOption) {}
2 changes: 1 addition & 1 deletion trace/trace.go
Original file line number Diff line number Diff line change
@@ -357,7 +357,7 @@ type Span interface {
// additional call to SetStatus is required if the Status of the Span should
// be set to Error, as this method does not change the Span status. If this
// span is not being recorded or err is nil then this method does nothing.
RecordError(err error, options ...EventOption)
RecordError(err error, options ...ErrorOption)
Copy link
Member

Choose a reason for hiding this comment

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

I think changing this may warrant a major release.

Copy link
Author

Choose a reason for hiding this comment

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

I think there must be an argument covariance/contravariance case I've missed here. I'll see if I can find a way of doing it and keeping full backwards compat.


// SpanContext returns the SpanContext of the Span. The returned SpanContext
// is usable even after the End method has been called for the Span.