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

Jump to bottom

Add WithStatus option for use with RecordError #2887

Closed
wants to merge 1 commit into from
Closed

Add WithStatus option for use with RecordError #2887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
dmathieu marked this conversation as resolved.
Show resolved Hide resolved

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

### Added
Expand Down
16 changes: 13 additions & 3 deletions bridge/opentracing/internal/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions internal/global/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down
25 changes: 16 additions & 9 deletions sdk/trace/span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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) {}
Expand Down
26 changes: 22 additions & 4 deletions sdk/trace/trace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down
68 changes: 57 additions & 11 deletions trace/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
}

Expand Down
5 changes: 1 addition & 4 deletions trace/noop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down
2 changes: 1 addition & 1 deletion trace/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down