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 trace.WithStatus option for span.RecordError and trace.WithStatusOnPanic option for span.End #5762

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
4 changes: 4 additions & 0 deletions CHANGELOG.md
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 `trace.WithErrorStatus` option as `trace.SpanEndOption` and `trace.EventOption`, which sets span's status to error. (#1677)

### Fixed

- Fix memory leak in the global `MeterProvider` when identical instruments are repeatedly created. (#5754)
Expand Down
12 changes: 11 additions & 1 deletion sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,12 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) {
if recovered := recover(); recovered != nil {
// Record but don't stop the panic.
defer panic(recovered)
recoveredStr := fmt.Sprint(recovered)

opts := []trace.EventOption{
trace.WithAttributes(
semconv.ExceptionType(typeStr(recovered)),
semconv.ExceptionMessage(fmt.Sprint(recovered)),
semconv.ExceptionMessage(recoveredStr),
),
}

Expand All @@ -409,6 +411,10 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) {
))
}

if config.ErrorStatus() {
s.SetStatus(codes.Error, recoveredStr)
}

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

Expand Down Expand Up @@ -466,6 +472,10 @@ func (s *recordingSpan) RecordError(err error, opts ...trace.EventOption) {
))
}

if c.ErrorStatus() {
s.SetStatus(codes.Error, err.Error())
}

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

Expand Down
74 changes: 74 additions & 0 deletions sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1299,6 +1299,54 @@ func TestRecordErrorWithStackTrace(t *testing.T) {
assert.Truef(t, strings.HasPrefix(gotStackTraceFunctionName[3], "go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).RecordError"), "%q not prefixed with go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).RecordError", gotStackTraceFunctionName[3])
}

func TestRecordErrorWithErrorStatus(t *testing.T) {
err := ottest.NewTestError("test error")
typ := "go.opentelemetry.io/otel/sdk/internal/internaltest.TestError"
msg := "test error"

te := NewTestExporter()
tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty()))
span := startSpan(tp, "RecordError")

errTime := time.Now()
span.RecordError(err, trace.WithTimestamp(errTime), trace.WithErrorStatus(true))

got, err := endSpan(te, span)
if err != nil {
t.Fatal(err)
}

want := &snapshot{
spanContext: trace.NewSpanContext(trace.SpanContextConfig{
TraceID: tid,
TraceFlags: 0x1,
}),
parent: sc.WithRemote(true),
name: "span0",
status: Status{Code: codes.Error, Description: msg},
spanKind: trace.SpanKindInternal,
events: []Event{
{
Name: semconv.ExceptionEventName,
Time: errTime,
Attributes: []attribute.KeyValue{
semconv.ExceptionType(typ),
semconv.ExceptionMessage(msg),
},
},
},
instrumentationScope: instrumentation.Scope{Name: "RecordError"},
}

assert.Equal(t, got.spanContext, want.spanContext)
assert.Equal(t, got.parent, want.parent)
assert.Equal(t, got.name, want.name)
assert.Equal(t, got.status, want.status)
assert.Equal(t, got.spanKind, want.spanKind)
assert.Equal(t, got.events[0].Attributes[0].Value.AsString(), want.events[0].Attributes[0].Value.AsString())
assert.Equal(t, got.events[0].Attributes[1].Value.AsString(), want.events[0].Attributes[1].Value.AsString())
}

func TestRecordErrorNil(t *testing.T) {
te := NewTestExporter()
tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty()))
Expand Down Expand Up @@ -1532,6 +1580,32 @@ func TestSpanCapturesPanicWithStackTrace(t *testing.T) {
assert.Truef(t, strings.HasPrefix(gotStackTraceFunctionName[3], "go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End"), "%q not prefixed with go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End", gotStackTraceFunctionName[3])
}

func TestSpanCapturesPanicWithErrorStatus(t *testing.T) {
err := errors.New("error message")
typ := "*errors.errorString"
msg := "error message"

te := NewTestExporter()
tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty()))
_, span := tp.Tracer("CatchPanic").Start(
context.Background(),
"span",
)

f := func() {
defer span.End(trace.WithErrorStatus(true))
panic(err)
}
require.PanicsWithError(t, msg, f)
spans := te.Spans()
require.Len(t, spans, 1)
require.Equal(t, Status{Code: codes.Error, Description: msg}, spans[0].Status())
require.Len(t, spans[0].Events(), 1)
assert.Equal(t, spans[0].Events()[0].Name, semconv.ExceptionEventName)
assert.Equal(t, spans[0].Events()[0].Attributes[0].Value.AsString(), typ)
assert.Equal(t, spans[0].Events()[0].Attributes[1].Value.AsString(), msg)
}

func TestReadOnlySpan(t *testing.T) {
kv := attribute.String("foo", "bar")

Expand Down
49 changes: 40 additions & 9 deletions trace/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,13 @@ func (fn tracerOptionFunc) apply(cfg TracerConfig) TracerConfig {

// SpanConfig is a group of options for a Span.
type SpanConfig struct {
attributes []attribute.KeyValue
timestamp time.Time
links []Link
newRoot bool
spanKind SpanKind
stackTrace bool
attributes []attribute.KeyValue
timestamp time.Time
links []Link
newRoot bool
spanKind SpanKind
stackTrace bool
errorStatus bool
}

// Attributes describe the associated qualities of a Span.
Expand Down Expand Up @@ -95,6 +96,11 @@ func (cfg *SpanConfig) SpanKind() SpanKind {
return cfg.spanKind
}

// ErrorStatus checks whether setting error status is enabled.
func (cfg *SpanConfig) ErrorStatus() bool {
return cfg.errorStatus
}

// NewSpanStartConfig applies all the options to a returned SpanConfig.
// No validation is performed on the returned SpanConfig (e.g. no uniqueness
// checking or bounding of data), it is left to the SDK to perform this
Expand Down Expand Up @@ -139,9 +145,10 @@ type SpanEndOption interface {

// EventConfig is a group of options for an Event.
type EventConfig struct {
attributes []attribute.KeyValue
timestamp time.Time
stackTrace bool
attributes []attribute.KeyValue
timestamp time.Time
stackTrace bool
errorStatus bool
}

// Attributes describe the associated qualities of an Event.
Expand All @@ -159,6 +166,11 @@ func (cfg *EventConfig) StackTrace() bool {
return cfg.stackTrace
}

// ErrorStatus checks whether setting error status is enabled.
func (cfg *EventConfig) ErrorStatus() bool {
return cfg.errorStatus
}

// NewEventConfig applies all the EventOptions to a returned EventConfig. If no
// timestamp option is passed, the returned EventConfig will have a Timestamp
// set to the call time, otherwise no validation is performed on the returned
Expand Down Expand Up @@ -269,6 +281,25 @@ func WithStackTrace(b bool) SpanEndEventOption {
return stackTraceOption(b)
}

type errorStatusOption bool

func (o errorStatusOption) applyEvent(c EventConfig) EventConfig {
c.errorStatus = bool(o)
return c
}

func (o errorStatusOption) applySpanEnd(c SpanConfig) SpanConfig {
c.errorStatus = bool(o)
return c
}

var _ SpanEndEventOption = errorStatusOption(true)

// WithErrorStatus sets the flag to set span's status to error if error or panic is occurred.
func WithErrorStatus(b bool) SpanEndEventOption {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the boolean? If we call the option, we want it enabled. Otherwise, we just don't add the parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yes, you are right.
I wanted to make it consistent with other options like WithStackTrace

func WithStackTrace(b bool) SpanEndEventOption {

However, I am not sure if it's acceptable to use options like:
span.RecordError(err, trace.WithStackTrace(true), trace.WithErrorStatus())

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I tend to think options with a boolean don't need that parameter. But for consistency, we could maybe still have it here.
Let's wait for other opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally I thought about splitting WithErrorStatus option:

  • WithStatus for span.RecordError call.
  • WithStatusOnPanic for span.End call.
    Because it may be not clear what means WithErrorStatus option when you call span.End.

But that also creates inconsistency with WithStackTrace option 🙃

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 thinking the WithStatusOnPanic name would be nice, as it makes things clearer.

return errorStatusOption(b)
}

// WithLinks adds links to a Span. The links are added to the existing Span
// links, i.e. this does not overwrite. Links with invalid span context are ignored.
func WithLinks(links ...Link) SpanStartOption {
Expand Down
70 changes: 70 additions & 0 deletions trace/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,82 @@ func TestEndSpanConfig(t *testing.T) {
timestamp: timestamp,
},
},
{
[]SpanEndOption{
WithErrorStatus(true),
},
SpanConfig{
errorStatus: true,
},
},
}
for _, test := range tests {
assert.Equal(t, test.expected, NewSpanEndConfig(test.options...))
}
}

func TestEventConfig(t *testing.T) {
kv := attribute.String("key", "value")
timestamp := time.Unix(0, 0)

tests := []struct {
options []EventOption
expected EventConfig
}{
{
[]EventOption{
WithTimestamp(timestamp),
},
EventConfig{
timestamp: timestamp,
},
},
{
[]EventOption{
WithTimestamp(timestamp),
WithStackTrace(true),
},
EventConfig{
timestamp: timestamp,
stackTrace: true,
},
},
{
[]EventOption{
WithTimestamp(timestamp),
WithStackTrace(true),
},
EventConfig{
timestamp: timestamp,
stackTrace: true,
},
},
{
[]EventOption{
WithTimestamp(timestamp),
WithAttributes(kv),
},
EventConfig{
timestamp: timestamp,
attributes: []attribute.KeyValue{kv},
},
},
{
[]EventOption{
WithTimestamp(timestamp),
WithErrorStatus(true),
},
EventConfig{
timestamp: timestamp,
errorStatus: true,
},
},
}
for _, test := range tests {
assert.Equal(t, test.expected, NewEventConfig(test.options...))
}
}

func TestTracerConfig(t *testing.T) {
v1 := "semver:0.0.1"
v2 := "semver:1.0.0"
Expand Down
Loading