OpenTelemetry: Specify span kind and don't set error type for benign exception (#2048)

* Add specifying SpanKind for Server and Client

* Don't set error for benign exceptions

* Only check top level err for Benign, use To/FromHeader instead of new

Author: yuandrew

contrib/opentelemetry/tracing_interceptor.go

@@ -14,6 +14,7 @@ import (
 
 	"go.temporal.io/sdk/interceptor"
 	"go.temporal.io/sdk/log"
+	"go.temporal.io/sdk/temporal"
 )
 
 // DefaultTextMapPropagator is the default OpenTelemetry TextMapPropagator used
@@ -196,8 +197,17 @@ func (t *tracer) StartSpan(opts *interceptor.TracerStartSpanOptions) (intercepto
 		}
 	}
 
+	if opts.ToHeader && opts.FromHeader {
+		return nil, fmt.Errorf("cannot set both ToHeader and FromHeader for span")
+	}
+
+	spanKind := trace.SpanKindServer
+	if opts.ToHeader {
+		spanKind = trace.SpanKindClient
+	}
+
 	// Create span
-	span := t.options.SpanStarter(ctx, t.options.Tracer, opts.Operation+":"+opts.Name, trace.WithTimestamp(opts.Time))
+	span := t.options.SpanStarter(ctx, t.options.Tracer, opts.Operation+":"+opts.Name, trace.WithTimestamp(opts.Time), trace.WithSpanKind(spanKind))
 
 	// Set tags
 	if len(opts.Tags) > 0 {
@@ -241,12 +251,19 @@ type tracerSpan struct {
 }
 
 func (t *tracerSpan) Finish(opts *interceptor.TracerFinishSpanOptions) {
-	if opts.Error != nil {
+	t.RecordError(opts.Error)
+
+	if opts.Error != nil && !isBenignApplicationError(opts.Error) {
 		t.SetStatus(codes.Error, opts.Error.Error())
 	}
 	t.End()
 }
 
+func isBenignApplicationError(err error) bool {
+	appError, _ := err.(*temporal.ApplicationError)
+	return appError != nil && appError.Category() == temporal.ApplicationErrorCategoryBenign
+}
+
 type textMapCarrier map[string]string
 
 func (t textMapCarrier) Get(key string) string        { return t[key] }

contrib/opentelemetry/tracing_interceptor_test.go

@@ -2,15 +2,19 @@ package opentelemetry_test
 
 import (
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/otel/codes"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 	"go.opentelemetry.io/otel/trace"
 
 	"go.temporal.io/sdk/contrib/opentelemetry"
 	"go.temporal.io/sdk/interceptor"
 	"go.temporal.io/sdk/internal/interceptortest"
+	"go.temporal.io/sdk/temporal"
 )
 
 func TestSpanPropagation(t *testing.T) {
@@ -42,3 +46,109 @@ func spanChildren(spans []sdktrace.ReadOnlySpan, parentID trace.SpanID) (ret []*
 	}
 	return
 }
+
+func TestSpanKind(t *testing.T) {
+	tests := []struct {
+		operation    string
+		toHeader     bool
+		fromHeader   bool
+		expectedKind trace.SpanKind
+	}{
+		{
+			operation:    "StartWorkflow",
+			toHeader:     true,
+			fromHeader:   false,
+			expectedKind: trace.SpanKindClient,
+		},
+		{
+			operation:    "RunWorkflow",
+			toHeader:     false,
+			fromHeader:   true,
+			expectedKind: trace.SpanKindServer,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.operation, func(t *testing.T) {
+			rec := tracetest.NewSpanRecorder()
+			tracer, err := opentelemetry.NewTracer(opentelemetry.TracerOptions{
+				Tracer: sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(rec)).Tracer(""),
+			})
+			require.NoError(t, err)
+
+			span, err := tracer.StartSpan(&interceptor.TracerStartSpanOptions{
+				Operation:  tt.operation,
+				Name:       "test-span",
+				ToHeader:   tt.toHeader,
+				FromHeader: tt.fromHeader,
+			})
+			require.NoError(t, err)
+
+			span.Finish(&interceptor.TracerFinishSpanOptions{})
+
+			spans := rec.Ended()
+			require.Equal(t, len(spans), 1)
+
+			foundSpan := spans[0]
+			assert.Equal(t, tt.expectedKind, foundSpan.SpanKind(),
+				"Expected span kind %v but got %v for operation %s",
+				tt.expectedKind, foundSpan.SpanKind(), tt.operation)
+		})
+	}
+}
+
+func TestBenignErrorSpanStatus(t *testing.T) {
+	tests := []struct {
+		name         string
+		err          error
+		expectError  bool
+		expectStatus codes.Code
+	}{
+		{
+			name:         "benign application error should not set error status",
+			err:          temporal.NewApplicationErrorWithOptions("benign error", "TestType", temporal.ApplicationErrorOptions{Category: temporal.ApplicationErrorCategoryBenign}),
+			expectError:  false,
+			expectStatus: codes.Unset,
+		},
+		{
+			name:         "regular application error should set error status",
+			err:          temporal.NewApplicationError("regular error", "TestType"),
+			expectError:  true,
+			expectStatus: codes.Error,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rec := tracetest.NewSpanRecorder()
+			tracer, err := opentelemetry.NewTracer(opentelemetry.TracerOptions{
+				Tracer: sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(rec)).Tracer(""),
+			})
+			require.NoError(t, err)
+
+			span, err := tracer.StartSpan(&interceptor.TracerStartSpanOptions{
+				Operation: "TestOperation",
+				Name:      "TestSpan",
+				Time:      time.Now(),
+			})
+			require.NoError(t, err)
+
+			span.Finish(&interceptor.TracerFinishSpanOptions{
+				Error: tt.err,
+			})
+
+			// Check recorded spans
+			spans := rec.Ended()
+			require.Len(t, spans, 1)
+
+			recordedSpan := spans[0]
+			assert.Equal(t, tt.expectStatus, recordedSpan.Status().Code)
+
+			if tt.expectError {
+				assert.NotEmpty(t, recordedSpan.Status().Description)
+			} else {
+				assert.Empty(t, recordedSpan.Status().Description)
+			}
+		})
+	}
+}