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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Added

- Add the `trace.WithStatus` option for `span.RecordError`. (#5762)
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem to be compliant with the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#record-exception. I think that in order to add such option the specification should be updated.

Have you done a research what other languages are doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are absolutely right. RecordError (or RecordException as it is named in the specification) should be just a specialized version of AddEvent, so users should set status manually.
This behavior can also be seen in the Java SDK and Python SDK:
https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L444
https://github.com/open-telemetry/opentelemetry-python/blob/d5fb2c4189a561bd36186d19923373761d4b3e7a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L1014

It is a little bit boilerplate code on every place, where we record exception. But nothing ugly in this case, that would require changing specification.
For example python aiohttp lib:
https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py#L297

So, probably we could only add trace.WithStatusOnPanic(). There is nothing in the specification that explicitly prohibits such an option. I believe it would be useful for users to avoid having a separate defer for catching panics and setting the status.
@pellared WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I found comments from Ted Young in slack, which states:

It should be an option to set the status when recording the exception, kinda bummed to see that did not make it into the spec. We should add this, or add a convenience method which sets both

I'm gonna ask about this option in specification community.

- Add the`trace.WithStatusOnPanic` option for `span.End`. (#5762)

### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`. Exemplars can be disabled by setting `OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
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.ErrorStatusOnPanic() {
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.WithStatus())

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.WithStatusOnPanic())
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
68 changes: 54 additions & 14 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
errorStatusOnPanic 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
}

// ErrorStatusOnPanic checks whether setting error status on panic is enabled.
func (cfg *SpanConfig) ErrorStatusOnPanic() bool {
return cfg.errorStatusOnPanic
}

// 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 @@ -125,9 +131,9 @@ type SpanStartOption interface {
applySpanStart(SpanConfig) SpanConfig
}

type spanOptionFunc func(SpanConfig) SpanConfig
type spanStartOptionFunc func(SpanConfig) SpanConfig

func (fn spanOptionFunc) applySpanStart(cfg SpanConfig) SpanConfig {
func (fn spanStartOptionFunc) applySpanStart(cfg SpanConfig) SpanConfig {
return fn(cfg)
}

Expand All @@ -137,11 +143,18 @@ type SpanEndOption interface {
applySpanEnd(SpanConfig) SpanConfig
}

type spanEndOptionFunc func(config SpanConfig) SpanConfig

func (fn spanEndOptionFunc) applySpanEnd(cfg SpanConfig) SpanConfig {
return fn(cfg)
}

// 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 +172,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 All @@ -179,6 +197,12 @@ type EventOption interface {
applyEvent(EventConfig) EventConfig
}

type eventOptionFunc func(EventConfig) EventConfig

func (fn eventOptionFunc) applyEvent(cfg EventConfig) EventConfig {
return fn(cfg)
}

// SpanOption are options that can be used at both the beginning and end of a span.
type SpanOption interface {
SpanStartOption
Expand Down Expand Up @@ -269,10 +293,26 @@ func WithStackTrace(b bool) SpanEndEventOption {
return stackTraceOption(b)
}

// WithStatus sets the flag to set span's status to error.
func WithStatus() EventOption {
return eventOptionFunc(func(cfg EventConfig) EventConfig {
cfg.errorStatus = true
return cfg
})
}

// WithStatusOnPanic sets the flag to set span's status to error if panic is occurred.
func WithStatusOnPanic() SpanEndOption {
return spanEndOptionFunc(func(cfg SpanConfig) SpanConfig {
cfg.errorStatusOnPanic = true
return cfg
})
}

// 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 {
return spanOptionFunc(func(cfg SpanConfig) SpanConfig {
return spanStartOptionFunc(func(cfg SpanConfig) SpanConfig {
cfg.links = append(cfg.links, links...)
return cfg
})
Expand All @@ -282,15 +322,15 @@ func WithLinks(links ...Link) SpanStartOption {
// existing parent span context will be ignored when defining the Span's trace
// identifiers.
func WithNewRoot() SpanStartOption {
return spanOptionFunc(func(cfg SpanConfig) SpanConfig {
return spanStartOptionFunc(func(cfg SpanConfig) SpanConfig {
cfg.newRoot = true
return cfg
})
}

// WithSpanKind sets the SpanKind of a Span.
func WithSpanKind(kind SpanKind) SpanStartOption {
return spanOptionFunc(func(cfg SpanConfig) SpanConfig {
return spanStartOptionFunc(func(cfg SpanConfig) SpanConfig {
cfg.spanKind = kind
return cfg
})
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{
WithStatusOnPanic(),
},
SpanConfig{
errorStatusOnPanic: 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),
WithStatus(),
},
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