From 9adf9835543033e6df2324efffcccb0f31c68135 Mon Sep 17 00:00:00 2001
From: Jack She <jackshe2702@gmail.com>
Date: Wed, 3 Jan 2024 03:16:24 +1100
Subject: [PATCH] add nil and recording check to span.RecordError (#1566)

Co-authored-by: Damien Mathieu <42@dmathieu.com>
---
 module/apmotel/span.go      |  4 ++
 module/apmotel/span_test.go | 73 ++++++++++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/module/apmotel/span.go b/module/apmotel/span.go
index c14fa1472..bcc47088e 100644
--- a/module/apmotel/span.go
+++ b/module/apmotel/span.go
@@ -129,6 +129,10 @@ func (s *span) IsRecording() bool {
 }
 
 func (s *span) RecordError(err error, opts ...trace.EventOption) {
+	if s == nil || err == nil || !s.IsRecording() {
+		return
+	}
+
 	opts = append(opts, trace.WithAttributes(
 		semconv.ExceptionType(typeStr(err)),
 		semconv.ExceptionMessage(err.Error()),
diff --git a/module/apmotel/span_test.go b/module/apmotel/span_test.go
index 47c3a3749..8924cdbcd 100644
--- a/module/apmotel/span_test.go
+++ b/module/apmotel/span_test.go
@@ -702,25 +702,64 @@ func TestSpanRecording(t *testing.T) {
 }
 
 func TestSpanRecordError(t *testing.T) {
-	tp, err := NewTracerProvider()
-	assert.NoError(t, err)
-	tracer := newTracer(tp.(*tracerProvider))
-	_, s := tracer.Start(context.Background(), "mySpan")
-
-	assert.Equal(t, []event(nil), s.(*span).events)
-
-	now := time.Now()
-	s.RecordError(errors.New("test"), trace.WithTimestamp(now))
-	assert.Equal(t, []event{
-		event{
-			Name: "exception",
-			Attributes: []attribute.KeyValue{
-				attribute.String("exception.type", "*errors.errorString"),
-				attribute.String("exception.message", "test"),
+	for _, tt := range []struct {
+		name              string
+		getSpan           func(context.Context, trace.Tracer) trace.Span
+		err               error
+		getExpectedEvents func(time.Time) []event
+	}{
+		{
+			name: "with a valid error",
+			getSpan: func(ctx context.Context, t trace.Tracer) trace.Span {
+				_, s := t.Start(context.Background(), "mySpan")
+				return s
+			},
+			err: errors.New("test"),
+			getExpectedEvents: func(t time.Time) []event {
+				return []event{{
+					Name: "exception",
+					Attributes: []attribute.KeyValue{
+						attribute.String("exception.type", "*errors.errorString"),
+						attribute.String("exception.message", "test"),
+					},
+					Time: t,
+				}}
 			},
-			Time: now,
 		},
-	}, s.(*span).events)
+		{
+			name: "with a nil error",
+			getSpan: func(ctx context.Context, t trace.Tracer) trace.Span {
+				_, s := t.Start(context.Background(), "mySpan")
+				return s
+			},
+			err:               nil,
+			getExpectedEvents: func(t time.Time) []event { return []event(nil) },
+		},
+		{
+			name: "with a non recording span",
+			getSpan: func(ctx context.Context, t trace.Tracer) trace.Span {
+				return &span{
+					tx: &apm.Transaction{},
+				}
+			},
+			err:               errors.New("test"),
+			getExpectedEvents: func(t time.Time) []event { return []event(nil) },
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			tp, err := NewTracerProvider()
+			assert.NoError(t, err)
+			tracer := newTracer(tp.(*tracerProvider))
+			s := tt.getSpan(context.Background(), tracer)
+
+			assert.Equal(t, []event(nil), s.(*span).events)
+
+			now := time.Now()
+			s.RecordError(tt.err, trace.WithTimestamp(now))
+
+			assert.Equal(t, tt.getExpectedEvents(now), s.(*span).events)
+		})
+	}
 }
 
 func TestSpanSetStatus(t *testing.T) {