From 94681a0c3fe1b983166b3004a01cfa84fedb65a3 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 2 Sep 2024 23:14:36 +0300 Subject: [PATCH 1/7] add WithErrorStatus to SpanEndOption and EventOption --- sdk/trace/span.go | 12 ++++++- sdk/trace/trace_test.go | 74 +++++++++++++++++++++++++++++++++++++++++ trace/config.go | 49 ++++++++++++++++++++++----- trace/config_test.go | 70 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 10 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 4945f508303..81bd832cd3d 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -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), ), } @@ -409,6 +411,10 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) { )) } + if config.ErrorStatus() { + s.SetStatus(codes.Error, recoveredStr) + } + s.addEvent(semconv.ExceptionEventName, opts...) } @@ -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...) } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 87247d1f167..40b9f10253c 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -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())) @@ -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") diff --git a/trace/config.go b/trace/config.go index 273d58e0014..a217dad9193 100644 --- a/trace/config.go +++ b/trace/config.go @@ -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. @@ -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 @@ -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. @@ -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 @@ -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 spans' status if error or panic is occurred. +func WithErrorStatus(b bool) SpanEndEventOption { + 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 { diff --git a/trace/config_test.go b/trace/config_test.go index 9a613ace2c9..460323a07bd 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -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" From ae4723d6f1e4117a06e7f4202afc7b76d6c74b9c Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 2 Sep 2024 23:36:43 +0300 Subject: [PATCH 2/7] fix grammar and add changelog --- CHANGELOG.md | 4 ++++ trace/config.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8c72034500..a3563c618cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/trace/config.go b/trace/config.go index a217dad9193..659e5a99b25 100644 --- a/trace/config.go +++ b/trace/config.go @@ -295,7 +295,7 @@ func (o errorStatusOption) applySpanEnd(c SpanConfig) SpanConfig { var _ SpanEndEventOption = errorStatusOption(true) -// WithErrorStatus sets the flag to set spans' status if error or panic is occurred. +// WithErrorStatus sets the flag to set span's status to error if error or panic is occurred. func WithErrorStatus(b bool) SpanEndEventOption { return errorStatusOption(b) } From e95733970e32efa3a1de6db3bb83942df4ec4a72 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Thu, 12 Sep 2024 22:59:03 +0300 Subject: [PATCH 3/7] update changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4146d20106e..7e40678a17d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. (#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) @@ -19,7 +23,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added -- Add `trace.WithErrorStatus` option as `trace.SpanEndOption` and `trace.EventOption`, which sets span's status to error. (#5762) - Support `OTEL_EXPORTER_OTLP_LOGS_INSECURE` and `OTEL_EXPORTER_OTLP_INSECURE` environments in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#5739) - The `WithResource` option for `NewMeterProvider` now merges the provided resources with the ones from environment variables. (#5773) - The `WithResource` option for `NewLoggerProvider` now merges the provided resources with the ones from environment variables. (#5773) From eb010b2534f79f325514a91fd99c37029a728155 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Fri, 13 Sep 2024 23:52:31 +0300 Subject: [PATCH 4/7] rename methods --- sdk/trace/span.go | 2 +- sdk/trace/trace_test.go | 4 +-- trace/config.go | 69 +++++++++++++++++++++++------------------ trace/config_test.go | 4 +-- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 81bd832cd3d..75e3becef4b 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -411,7 +411,7 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) { )) } - if config.ErrorStatus() { + if config.ErrorStatusOnPanic() { s.SetStatus(codes.Error, recoveredStr) } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 40b9f10253c..ba44b1ebc5a 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1309,7 +1309,7 @@ func TestRecordErrorWithErrorStatus(t *testing.T) { span := startSpan(tp, "RecordError") errTime := time.Now() - span.RecordError(err, trace.WithTimestamp(errTime), trace.WithErrorStatus(true)) + span.RecordError(err, trace.WithTimestamp(errTime), trace.WithStatus()) got, err := endSpan(te, span) if err != nil { @@ -1593,7 +1593,7 @@ func TestSpanCapturesPanicWithErrorStatus(t *testing.T) { ) f := func() { - defer span.End(trace.WithErrorStatus(true)) + defer span.End(trace.WithStatusOnPanic()) panic(err) } require.PanicsWithError(t, msg, f) diff --git a/trace/config.go b/trace/config.go index 659e5a99b25..2413f503a9f 100644 --- a/trace/config.go +++ b/trace/config.go @@ -55,13 +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 - errorStatus 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. @@ -96,9 +96,9 @@ func (cfg *SpanConfig) SpanKind() SpanKind { return cfg.spanKind } -// ErrorStatus checks whether setting error status is enabled. -func (cfg *SpanConfig) ErrorStatus() bool { - return cfg.errorStatus +// 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. @@ -131,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) } @@ -143,6 +143,12 @@ 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 @@ -191,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 @@ -281,29 +293,26 @@ 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 +// 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 + }) } -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 { - return errorStatusOption(b) +// 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 }) @@ -313,7 +322,7 @@ 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 }) @@ -321,7 +330,7 @@ func WithNewRoot() SpanStartOption { // 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 }) diff --git a/trace/config_test.go b/trace/config_test.go index 460323a07bd..f7b7fa516b1 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -205,10 +205,10 @@ func TestEndSpanConfig(t *testing.T) { }, { []SpanEndOption{ - WithErrorStatus(true), + WithStatusOnPanic(), }, SpanConfig{ - errorStatus: true, + errorStatusOnPanic: true, }, }, } From a36a6b6f48a84294fbba31f2ac52512d7b863d6d Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Fri, 13 Sep 2024 23:58:19 +0300 Subject: [PATCH 5/7] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdd4f204fd7..1dcea2fa800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added -- Add `trace.WithErrorStatus` option as `trace.SpanEndOption` and `trace.EventOption`, which sets span's status to error. (#5762) +- Add `trace.WithStatus` option for `span.RecordError` and `trace.WithStatusOnPanic` option for `span.End`. (#5762) ### Changed From c0f739db265e44d14e504c95acadc640567b9175 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Sat, 14 Sep 2024 00:00:32 +0300 Subject: [PATCH 6/7] fix test --- trace/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trace/config_test.go b/trace/config_test.go index f7b7fa516b1..aaccff24628 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -266,7 +266,7 @@ func TestEventConfig(t *testing.T) { { []EventOption{ WithTimestamp(timestamp), - WithErrorStatus(true), + WithStatus(), }, EventConfig{ timestamp: timestamp, From 84fa504ed200f9d4d4eca072924a4b736809bf8d Mon Sep 17 00:00:00 2001 From: Anton Manakin <45166364+amanakin@users.noreply.github.com> Date: Mon, 16 Sep 2024 12:19:30 +0300 Subject: [PATCH 7/7] update changelog Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dcea2fa800..750d9addde1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added -- Add `trace.WithStatus` option for `span.RecordError` and `trace.WithStatusOnPanic` option for `span.End`. (#5762) +- Add the `trace.WithStatus` option for `span.RecordError`. (#5762) +- Add the`trace.WithStatusOnPanic` option for `span.End`. (#5762) ### Changed