OpenTelemetry: Specify span kind and don't set error type for benign exception

contrib/opentelemetry/tracing_interceptor.go

@@ -3,6 +3,7 @@ package opentelemetry
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"go.opentelemetry.io/otel"
@@ -14,6 +15,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 +198,13 @@ func (t *tracer) StartSpan(opts *interceptor.TracerStartSpanOptions) (intercepto
 		}
 	}
 
+	spanKind := trace.SpanKindServer
+	if opts.Outbound {
+		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 +248,22 @@ type tracerSpan struct {
 }
 
 func (t *tracerSpan) Finish(opts *interceptor.TracerFinishSpanOptions) {
-	if opts.Error != nil {
+	if opts.Error != nil && !isBenignApplicationError(opts.Error) {
 		t.SetStatus(codes.Error, opts.Error.Error())
 	}
 	t.End()
 }
 
+func isBenignApplicationError(err error) bool {
+	var appErr *temporal.ApplicationError
+	if temporal.IsApplicationError(err) {
+		if errors.As(err, &appErr) {
+			return appErr.Category() == temporal.ApplicationErrorCategoryBenign
+		}
+	}
+	return false
+}
+
 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,105 @@ func spanChildren(spans []sdktrace.ReadOnlySpan, parentID trace.SpanID) (ret []*
 	}
 	return
 }
+
+func TestSpanKind(t *testing.T) {
+	tests := []struct {
+		operation    string
+		outbound     bool
+		expectedKind trace.SpanKind
+	}{
+		{
+			operation:    "StartWorkflow",
+			outbound:     true,
+			expectedKind: trace.SpanKindClient,
+		},
+		{
+			operation:    "RunWorkflow",
+			outbound:     false,
+			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",
+				Outbound:  tt.outbound,
+			})
+			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 (outbound=%v)",
+				tt.expectedKind, foundSpan.SpanKind(), tt.operation, tt.outbound)
+		})
+	}
+}
+
+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)
+			}
+		})
+	}
+}

contrib/opentracing/interceptor.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"go.temporal.io/sdk/temporal"
 
 	"github.com/opentracing/opentracing-go"
 
@@ -164,9 +165,19 @@ type tracerSpanRef struct{ opentracing.SpanContext }
 type tracerSpan struct{ opentracing.Span }
 
 func (t *tracerSpan) Finish(opts *interceptor.TracerFinishSpanOptions) {
-	if opts.Error != nil {
+	if opts.Error != nil && !isBenignApplicationError(opts.Error) {
 		// Standard tag that can be bridged to OpenTelemetry
 		t.SetTag("error", "true")
 	}
 	t.Span.Finish()
 }
+
+func isBenignApplicationError(err error) bool {
+	var appErr *temporal.ApplicationError
+	if temporal.IsApplicationError(err) {
+		if errors.As(err, &appErr) {
+			return appErr.Category() == temporal.ApplicationErrorCategoryBenign
+		}
+	}
+	return false
+}