Changed signature of the Span RecordError, replace EventOption by E…

…rrorOption (open-telemetry#1677) * Change signature of the Span `RecordError, replace EventOption by ErrorOption * Add WithEventOpts, WithErrorStatus * Set status when WithErrorStatus is passed to RecordError Signed-off-by: lastchiliarch <[email protected]>
lastchiliarch · Apr 26, 2021 · 364ad1e · 364ad1e
2 parents c1573c2 + d3b58b6
commit 364ad1e
Show file tree

Hide file tree

Showing 10 changed files with 191 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -141,7 +141,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Added
 
-- Added `WithStatus` event option to set Span status at the same time when call RecordError. (#1677)
 - Added `resource.Default()` for use with meter and tracer providers. (#1507)
 - `AttributePerEventCountLimit` and `AttributePerLinkCountLimit` for `SpanLimits`. (#1535)
 - Added `Keys()` method to `propagation.TextMapCarrier` and `propagation.HeaderCarrier` to adapt `http.Header` to this interface. (#1544)
@@ -162,6 +161,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Changed
 
+- Changed signature of the Span `RecordError` , replace EventOption by ErrorOption
 - Replaced interface `oteltest.SpanRecorder` with its existing implementation
   `StandardSpanRecorder`. (#1542)
 - Default span limit values to 128. (#1535)

diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go
@@ -245,7 +245,7 @@ func (s *MockSpan) End(options ...trace.SpanOption) {
 	s.mockTracer.FinishedSpans = append(s.mockTracer.FinishedSpans, s)
 }
 
-func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
+func (s *MockSpan) RecordError(err error, options ...trace.ErrorOption) {
 	if err == nil {
 		return // no-op on nil error
 	}
@@ -255,11 +255,13 @@ func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
 	}
 
 	s.SetStatus(codes.Error, "")
-	opts = append(opts, trace.WithAttributes(
+	c := trace.NewErrorConfig(options...)
+	eventOpts := c.EventOpts
+	eventOpts = append(eventOpts, trace.WithAttributes(
 		semconv.ExceptionTypeKey.String(reflect.TypeOf(err).String()),
 		semconv.ExceptionMessageKey.String(err.Error()),
 	))
-	s.AddEvent(semconv.ExceptionEventName, opts...)
+	s.AddEvent(semconv.ExceptionEventName, eventOpts...)
 }
 
 func (s *MockSpan) Tracer() trace.Tracer {

diff --git a/oteltest/span.go b/oteltest/span.go
@@ -75,7 +75,7 @@ func (s *Span) End(opts ...trace.SpanOption) {
 }
 
 // RecordError records an error as an exception Span event.
-func (s *Span) RecordError(err error, opts ...trace.EventOption) {
+func (s *Span) RecordError(err error, options ...trace.ErrorOption) {
 	if err == nil || s.ended {
 		return
 	}
@@ -86,12 +86,14 @@ func (s *Span) RecordError(err error, opts ...trace.EventOption) {
 		errTypeString = errType.String()
 	}
 
-	opts = append(opts, trace.WithAttributes(
+	c := trace.NewErrorConfig(options...)
+	eventOpts := c.EventOpts
+	eventOpts = append(eventOpts, trace.WithAttributes(
 		semconv.ExceptionTypeKey.String(errTypeString),
 		semconv.ExceptionMessageKey.String(err.Error()),
 	))
 
-	s.AddEvent(semconv.ExceptionEventName, opts...)
+	s.AddEvent(semconv.ExceptionEventName, eventOpts...)
 }
 
 // AddEvent adds an event to s.

diff --git a/oteltest/span_test.go b/oteltest/span_test.go
@@ -157,7 +157,7 @@ func TestSpan(t *testing.T) {
 				e.Expect(ok).ToBeTrue()
 
 				testTime := time.Now()
-				subject.RecordError(s.err, trace.WithTimestamp(testTime))
+				subject.RecordError(s.err, trace.WithEventOpts(trace.WithTimestamp(testTime)))
 
 				expectedEvents := []oteltest.Event{{
 					Timestamp: testTime,

diff --git a/sdk/trace/span.go b/sdk/trace/span.go
@@ -248,20 +248,22 @@ func (s *span) End(options ...trace.SpanOption) {
 // This method does not change the Span status in default
 // To change Span status, pass `WithStatus` as options.
 // This method does nothing If this span is not being recorded or err is nil.
-func (s *span) RecordError(err error, opts ...trace.EventOption) {
+func (s *span) RecordError(err error, options ...trace.ErrorOption) {
 	if s == nil || err == nil || !s.IsRecording() {
 		return
 	}
-	c := trace.NewEventConfig(opts...)
-	if c.WithStatus {
-		s.SetStatus(codes.Error, "")
+
+	c := trace.NewErrorConfig(options...)
+	if c.Code != codes.Unset {
+		s.SetStatus(c.Code, c.Message)
 	}
 
-	opts = append(opts, trace.WithAttributes(
+	eventOpts := c.EventOpts
+	eventOpts = append(eventOpts, trace.WithAttributes(
 		semconv.ExceptionTypeKey.String(typeStr(err)),
 		semconv.ExceptionMessageKey.String(err.Error()),
 	))
-	s.addEvent(semconv.ExceptionEventName, opts...)
+	s.addEvent(semconv.ExceptionEventName, eventOpts...)
 }
 
 func typeStr(i interface{}) string {

diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
@@ -1101,7 +1101,7 @@ func TestRecordError(t *testing.T) {
 		span := startSpan(tp, "RecordError")
 
 		errTime := time.Now()
-		span.RecordError(s.err, trace.WithTimestamp(errTime))
+		span.RecordError(s.err, trace.WithEventOpts(trace.WithTimestamp(errTime)))
 
 		got, err := endSpan(te, span)
 		if err != nil {
@@ -1164,14 +1164,34 @@ func TestRecordErrorNil(t *testing.T) {
 	}
 }
 
-func TestRecordErrorWithStatus(t *testing.T) {
+func TestRecordErrorOptions(t *testing.T) {
 	errTime := time.Now()
 	scenarios := []struct {
-		err  error
-		want *SpanSnapshot
+		err     error
+		options []trace.ErrorOption
+		want    *SpanSnapshot
 	}{
+		{
+			err:     nil,
+			options: []trace.ErrorOption{},
+			want: &SpanSnapshot{
+				SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
+					TraceID:    tid,
+					TraceFlags: 0x1,
+				}),
+				Parent:                 sc.WithRemote(true),
+				Name:                   "span0",
+				StatusCode:             codes.Unset,
+				SpanKind:               trace.SpanKindInternal,
+				InstrumentationLibrary: instrumentation.Library{Name: "RecordError"},
+			},
+		},
 		{
 			err: ottest.NewTestError("test error"),
+			options: []trace.ErrorOption{
+				trace.WithErrorStatus(codes.Error, "test error"),
+				trace.WithEventOpts(trace.WithTimestamp(errTime)),
+			},
 			want: &SpanSnapshot{
 				SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 					TraceID:    tid,
@@ -1191,20 +1211,42 @@ func TestRecordErrorWithStatus(t *testing.T) {
 						},
 					},
 				},
+				StatusMessage:          "test error",
 				InstrumentationLibrary: instrumentation.Library{Name: "RecordError"},
 			},
 		},
 		{
-			err: nil,
+			err: ottest.NewTestError("test error"),
+			options: []trace.ErrorOption{
+				trace.WithErrorStatus(codes.Error, "test error"),
+				trace.WithEventOpts(
+					trace.WithTimestamp(errTime),
+					trace.WithAttributes(attribute.String("key1", "value1")),
+					trace.WithAttributes(attribute.String("key2", "value2")),
+				),
+			},
 			want: &SpanSnapshot{
 				SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 					TraceID:    tid,
 					TraceFlags: 0x1,
 				}),
-				Parent:                 sc.WithRemote(true),
-				Name:                   "span0",
-				StatusCode:             codes.Unset,
-				SpanKind:               trace.SpanKindInternal,
+				Parent:     sc.WithRemote(true),
+				Name:       "span0",
+				StatusCode: codes.Error,
+				SpanKind:   trace.SpanKindInternal,
+				MessageEvents: []trace.Event{
+					{
+						Name: semconv.ExceptionEventName,
+						Time: errTime,
+						Attributes: []attribute.KeyValue{
+							attribute.String("key1", "value1"),
+							attribute.String("key2", "value2"),
+							semconv.ExceptionTypeKey.String("go.opentelemetry.io/otel/internal/internaltest.TestError"),
+							semconv.ExceptionMessageKey.String("test error"),
+						},
+					},
+				},
+				StatusMessage:          "test error",
 				InstrumentationLibrary: instrumentation.Library{Name: "RecordError"},
 			},
 		},
@@ -1215,7 +1257,7 @@ func TestRecordErrorWithStatus(t *testing.T) {
 		tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty()))
 
 		span := startSpan(tp, "RecordError")
-		span.RecordError(s.err, trace.WithTimestamp(errTime), trace.WithStatus(true))
+		span.RecordError(s.err, s.options...)
 		got, err := endSpan(te, span)
 		if err != nil {
 			t.Fatal(err)

diff --git a/trace/config.go b/trace/config.go
@@ -17,6 +17,8 @@ package trace
 import (
 	"time"
 
+	"go.opentelemetry.io/otel/codes"
+
 	"go.opentelemetry.io/otel/attribute"
 )
 
@@ -60,8 +62,6 @@ type SpanConfig struct {
 	NewRoot bool
 	// SpanKind is the role a Span has in a trace.
 	SpanKind SpanKind
-	// WithStatus is used to control whether or not set Span status when RecordError
-	WithStatus bool
 }
 
 // NewSpanConfig applies all the options to a returned SpanConfig.
@@ -206,14 +206,55 @@ func (i instrumentationVersionOption) ApplyTracer(config *TracerConfig) {
 
 func (instrumentationVersionOption) private() {}
 
-type withStatusSpanOption bool
+// ErrorConfig is a group of options for error.
+type ErrorConfig struct {
+	Code    codes.Code
+	Message string
+	// EventOpts will be passed to addEvent
+	EventOpts []EventOption
+}
+
+// ErrorOption applies an option to a ErrorConfig.
+type ErrorOption interface {
+	ApplyError(config *ErrorConfig)
+
+	// A private method to prevent users implementing the
+	// interface and so future additions to it will not
+	// violate compatibility.
+	private()
+}
+
+// NewErrorConfig applies all the options to a returned ErrorConfig.
+func NewErrorConfig(options ...ErrorOption) *ErrorConfig {
+	c := new(ErrorConfig)
+	for _, option := range options {
+		option.ApplyError(c)
+	}
+	return c
+}
+
+type statusErrorOption struct {
+	Code    codes.Code
+	Message string
+}
+
+func (o statusErrorOption) ApplyError(c *ErrorConfig) {
+	c.Code = o.Code
+	c.Message = o.Message
+}
+func (statusErrorOption) private() {}
+
+// WithErrorStatus set the error code and message when code is not codes.Unset
+func WithErrorStatus(code codes.Code, message string) ErrorOption {
+	return statusErrorOption{code, message}
+}
+
+type eventOptsErrorOption []EventOption
 
-func (o withStatusSpanOption) ApplySpan(c *SpanConfig)  { o.apply(c) }
-func (o withStatusSpanOption) ApplyEvent(c *SpanConfig) { o.apply(c) }
-func (withStatusSpanOption) private()                   {}
-func (o withStatusSpanOption) apply(c *SpanConfig)      { c.WithStatus = bool(o) }
+func (o eventOptsErrorOption) ApplyError(c *ErrorConfig) { c.EventOpts = o }
+func (eventOptsErrorOption) private()                    {}
 
-// WithStatus set the status at the same time when RecordError
-func WithStatus(o bool) LifeCycleOption {
-	return withStatusSpanOption(o)
+// WithEventOpts set event options, will be passed to addEvent
+func WithEventOpts(option ...EventOption) ErrorOption {
+	return eventOptsErrorOption(option)
 }