feat(tracing): OTel improvements from go-fan review

internal/proxy/handler.go

@@ -12,7 +12,6 @@ import (
 	"time"
 
 	"go.opentelemetry.io/otel/attribute"
-	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
 	oteltrace "go.opentelemetry.io/otel/trace"
 
 	"github.com/github/gh-aw-mcpg/internal/difc"
@@ -199,9 +198,11 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 		if denied, _ := guard.HandlePrePhaseError(err); denied != nil {
 			logHandler.Printf("[DIFC] Phase 2: BLOCKED %s %s — %s", r.Method, path, denied.EvalResult.Reason)
 			deniedErr := fmt.Errorf("DIFC policy violation: %s", denied.EvalResult.Reason)
-			difcSpan.AddEvent("difc.access_denied", oteltrace.WithAttributes(
-				attribute.String("reason", denied.EvalResult.Reason),
-			))
+			if difcSpan.IsRecording() {
+				difcSpan.AddEvent("difc.access_denied", oteltrace.WithAttributes(
+					attribute.String("reason", denied.EvalResult.Reason),
+				))
+			}
 			tracing.RecordSpanError(difcSpan, deniedErr, "access denied: "+denied.EvalResult.Reason)
 			writeDIFCForbidden(w, deniedErr.Error())
 			return
@@ -211,7 +212,9 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 		httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "resource labeling failed")
 		return
 	}
-	difcSpan.AddEvent("difc.pre_phases_complete")
+	if difcSpan.IsRecording() {
+		difcSpan.AddEvent("difc.pre_phases_complete")
+	}
 
 	// **Phase 3: Forward to upstream GitHub API**
 	clientAuth := r.Header.Get("Authorization")
@@ -226,14 +229,16 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 		resp, respBody = h.forwardAndReadBody(w, fwdCtx, r.Method, path, nil, "", clientAuth)
 	}
 	if resp != nil {
-		fwdSpan.SetAttributes(semconv.HTTPResponseStatusCodeKey.Int(resp.StatusCode))
+		fwdSpan.SetAttributes(tracing.HTTPResponseStatusCodeKey.Int(resp.StatusCode))
 		if rateLimited, resetHeader := rateLimitSignal(resp); rateLimited {
 			fwdSpan.SetAttributes(tracing.RateLimitHit.Bool(true))
-			eventAttrs := []attribute.KeyValue{}
-			if resetAt := githubhttp.ParseRateLimitResetHeader(resetHeader); !resetAt.IsZero() {
-				eventAttrs = append(eventAttrs, attribute.String("reset_at", resetAt.UTC().Format(time.RFC3339)))
+			if difcSpan.IsRecording() {
+				eventAttrs := []attribute.KeyValue{}
+				if resetAt := githubhttp.ParseRateLimitResetHeader(resetHeader); !resetAt.IsZero() {
+					eventAttrs = append(eventAttrs, attribute.String("reset_at", resetAt.UTC().Format(time.RFC3339)))
+				}
+				difcSpan.AddEvent("rate_limit.detected", oteltrace.WithAttributes(eventAttrs...))
 			}
-			difcSpan.AddEvent("rate_limit.detected", oteltrace.WithAttributes(eventAttrs...))
 		}
 	}
 	if resp == nil {

internal/server/unified.go

@@ -438,9 +438,11 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		if denied, detailedErr := guard.HandlePrePhaseError(err); denied != nil {
 			logger.LogWarn("difc", "Access DENIED for agent %s to %s: %s",
 				agentID, denied.Resource.Description, denied.EvalResult.Reason)
-			toolSpan.AddEvent("difc.access_denied", oteltrace.WithAttributes(
-				attribute.String("reason", denied.EvalResult.Reason),
-			))
+			if toolSpan.IsRecording() {
+				toolSpan.AddEvent("difc.access_denied", oteltrace.WithAttributes(
+					attribute.String("reason", denied.EvalResult.Reason),
+				))
+			}
 			tracing.RecordSpanError(toolSpan, detailedErr, "access denied: "+denied.EvalResult.Reason)
 			httpStatusCode = 403
 			return mcp.NewErrorCallToolResult(detailedErr)
@@ -449,8 +451,9 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		httpStatusCode = 500
 		return mcp.NewErrorCallToolResult(fmt.Errorf("guard labeling failed: %w", err))
 	}
-	toolSpan.AddEvent("difc.pre_phases_complete")
-
+	if toolSpan.IsRecording() {
+		toolSpan.AddEvent("difc.pre_phases_complete")
+	}
 	// Add agent tags snapshot to context for enriched MCP backend logging (Phase 3).
 	ctx = context.WithValue(ctx, mcp.AgentTagsSnapshotContextKey, &mcp.AgentTagsSnapshot{
 		Secrecy:   difc.TagsToStrings(pre.AgentLabels.GetSecrecyTags()),
@@ -474,9 +477,10 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		// Transport errors (connection failure, JSON parse error, etc.) are not rate-limit
 		// events and must not affect the consecutive rate-limit counter. Leave the circuit
 		// breaker state unchanged so genuine rate-limit history is preserved.
-		// Use a generic error message for trace recording to avoid leaking internal details
-		// to trace backends; the full error is returned to the caller and logged separately.
-		tracing.RecordSpanError(execSpan, fmt.Errorf("tool execution failed"), "tool execution failed")
+		// Use RecordSpanErrorSafe to avoid leaking internal transport/parse error
+		// details to trace backends; the full error is returned to the caller and
+		// logged separately.
+		tracing.RecordSpanErrorSafe(execSpan, err, "tool execution failed")
 		httpStatusCode = 500
 		return mcp.NewErrorCallToolResult(err)
 	}
@@ -486,11 +490,13 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		cb.RecordRateLimit(resetAt)
 		execSpan.SetAttributes(tracing.RateLimitHit.Bool(true))
 		toolSpan.SetAttributes(tracing.RateLimitHit.Bool(true))
-		eventAttrs := []attribute.KeyValue{}
-		if !resetAt.IsZero() {
-			eventAttrs = append(eventAttrs, attribute.String("reset_at", resetAt.UTC().Format(time.RFC3339)))
+		if toolSpan.IsRecording() {
+			eventAttrs := []attribute.KeyValue{}
+			if !resetAt.IsZero() {
+				eventAttrs = append(eventAttrs, attribute.String("reset_at", resetAt.UTC().Format(time.RFC3339)))
+			}
+			toolSpan.AddEvent("rate_limit.detected", oteltrace.WithAttributes(eventAttrs...))
 		}
-		toolSpan.AddEvent("rate_limit.detected", oteltrace.WithAttributes(eventAttrs...))
 		tracing.RecordSpanErrorOnAll(errRateLimitExceeded, rateLimitExceededStatus, execSpan, toolSpan)
 		httpStatusCode = 429
 		// Preserve the original backend error text so the agent sees the actual upstream

internal/tracing/fanout.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"sync"
+	"time"
 
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
@@ -66,12 +67,32 @@ func (f *fanoutExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadO
 	})
 }
 
-// Shutdown shuts down each underlying exporter concurrently, collecting any
-// errors. All errors are joined and returned.
+// Shutdown shuts down each underlying exporter concurrently, giving each
+// exporter its own context derived from the parent's remaining deadline.
+// This prevents a slow or unresponsive exporter from consuming the entire
+// deadline and leaving insufficient time for other exporters to complete.
 func (f *fanoutExporter) Shutdown(ctx context.Context) error {
 	logTracing.Printf("fanoutExporter.Shutdown: shutting down %d backends", len(f.exporters))
+
+	// Capture the remaining time budget from the parent context once so that
+	// every goroutine starts with the same deadline, even if Go's scheduler
+	// delays some goroutines after others begin executing.
+	var remaining time.Duration
+	if deadline, ok := ctx.Deadline(); ok {
+		remaining = time.Until(deadline)
+	}
+
 	err := f.forEachExporter("Shutdown", func(e sdktrace.SpanExporter) error {
-		return e.Shutdown(ctx)
+		// When there is no deadline, use the parent context as-is so that
+		// parent cancellation still propagates.
+		if remaining <= 0 {
+			return e.Shutdown(ctx)
+		}
+		// Give each exporter a fresh, independent context with the remaining
+		// budget so one slow exporter cannot starve the others.
+		exporterCtx, cancel := context.WithTimeout(ctx, remaining)
+		defer cancel()
+		return e.Shutdown(exporterCtx)
 	})
 	logTracing.Printf("fanoutExporter.Shutdown: completed")
 	return err

internal/tracing/genai_attrs.go

@@ -1,38 +1,16 @@
 // Package tracing provides OpenTelemetry OTLP trace export for the MCP Gateway.
-// This file defines gen_ai semantic convention attribute keys per
-// https://opentelemetry.io/docs/specs/semconv/registry/attributes/gen-ai/
+// This file defines custom and non-semconv attribute keys for gen_ai and MCP spans.
+// The semconv-derived GenAI attribute keys (GenAIToolName, GenAIOperationName, etc.)
+// are defined in semconv.go, which is the single file that imports
+// go.opentelemetry.io/otel/semconv/v1.41.0.
 package tracing
 
-import (
-	"go.opentelemetry.io/otel/attribute"
-	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
-)
-
-// GenAI semantic convention attribute keys.
-// Most are aliases for the official OpenTelemetry gen_ai semconv constants
-// (semconv/v1.41.0), re-exported here for convenience. GenAISystem is the
-// exception: it was removed in semconv/v1.41.0 and is defined as a raw
-// attribute.Key to preserve wire compatibility.
-const (
-	// GenAISystem identifies the GenAI system family for MCP spans.
-	// gen_ai.system was removed from semconv/v1.41.0; the key string is preserved for compatibility.
-	GenAISystem = attribute.Key("gen_ai.system")
-
-	// GenAIToolName is the name of the tool utilized by the agent.
-	GenAIToolName = semconv.GenAIToolNameKey
+import "go.opentelemetry.io/otel/attribute"
 
-	// GenAIOperationName is the name of the operation being performed.
-	GenAIOperationName = semconv.GenAIOperationNameKey
-
-	// GenAIConversationID is the unique identifier for a conversation (session).
-	GenAIConversationID = semconv.GenAIConversationIDKey
-
-	// GenAIAgentName is the human-readable name of the GenAI agent.
-	GenAIAgentName = semconv.GenAIAgentNameKey
-
-	// GenAIAgentID is the unique identifier of the GenAI agent (server ID).
-	GenAIAgentID = semconv.GenAIAgentIDKey
-)
+// GenAISystem identifies the GenAI system family for MCP spans.
+// gen_ai.system was removed from semconv/v1.41.0; the key string is preserved for
+// wire compatibility with observability backends that still expect it.
+const GenAISystem = attribute.Key("gen_ai.system")
 
 // MCP-specific attribute keys (no gen_ai equivalent in the spec).
 const (

internal/tracing/http.go

@@ -11,7 +11,6 @@ import (
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/propagation"
-	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
 	oteltrace "go.opentelemetry.io/otel/trace"
 
 	"github.com/github/gh-aw-mcpg/internal/httputil"
@@ -58,11 +57,11 @@ func WrapHTTPHandler(next http.Handler, spanName string, extraAttrs ...attribute
 		}
 
 		attrs := append([]attribute.KeyValue{
-			semconv.HTTPRequestMethodKey.String(r.Method),
-			semconv.URLPathKey.String(r.URL.Path),
+			HTTPRequestMethodKey.String(r.Method),
+			URLPathKey.String(r.URL.Path),
 		}, extraAttrs...)
 		if route != "" {
-			attrs = append(attrs, semconv.HTTPRouteKey.String(route))
+			attrs = append(attrs, HTTPRouteKey.String(route))
 		}
 
 		ctx, span := t.Start(ctx, spanName,
@@ -75,7 +74,7 @@ func WrapHTTPHandler(next http.Handler, spanName string, extraAttrs ...attribute
 
 		srw := &statusResponseWriter{BaseResponseWriter: httputil.BaseResponseWriter{ResponseWriter: w, StatusCode: http.StatusOK}}
 		defer func() {
-			span.SetAttributes(semconv.HTTPResponseStatusCodeKey.Int(srw.StatusCode))
+			span.SetAttributes(HTTPResponseStatusCodeKey.Int(srw.StatusCode))
 			if srw.StatusCode >= 500 {
 				msg := http.StatusText(srw.StatusCode)
 				if msg == "" {

internal/tracing/provider.go

@@ -29,7 +29,6 @@ import (
 	"go.opentelemetry.io/otel/propagation"
 	"go.opentelemetry.io/otel/sdk/resource"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
-	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
 	"go.opentelemetry.io/otel/trace"
 	"go.opentelemetry.io/otel/trace/noop"
 
@@ -192,18 +191,18 @@ func InitProvider(ctx context.Context, cfg *config.TracingConfig) (*Provider, er
 		resource.WithTelemetrySDK(),
 		resource.WithContainer(),
 		resource.WithAttributes(
-			semconv.ServiceName(serviceName),
-			semconv.ServiceVersion(version.Get()),
+			ServiceName(serviceName),
+			ServiceVersion(version.Get()),
 		),
 		resource.WithProcessPID(),
 		resource.WithHost(),
 	)
 	if err != nil {
 		logTracing.Printf("Warning: failed to create OTEL resource: %v", err)
 		serviceResource := resource.NewWithAttributes(
-			semconv.SchemaURL,
-			semconv.ServiceName(serviceName),
-			semconv.ServiceVersion(version.Get()),
+			SchemaURL,
+			ServiceName(serviceName),
+			ServiceVersion(version.Get()),
 		)
 		// resource.New can return a best-effort resource alongside an error.
 		// Preserve detected attributes and only ensure service identity exists.
@@ -231,7 +230,13 @@ func InitProvider(ctx context.Context, cfg *config.TracingConfig) (*Provider, er
 	}
 
 	sdkTP := sdktrace.NewTracerProvider(
-		sdktrace.WithBatcher(exporter),
+		sdktrace.WithBatcher(exporter,
+			// Tune batch processor for gateway throughput: halve the default batch
+			// size (512→256) and reduce the flush interval (5s→2s) to lower tail
+			// latency for trace delivery without sacrificing export efficiency.
+			sdktrace.WithMaxExportBatchSize(256),
+			sdktrace.WithBatchTimeout(2*time.Second),
+		),
 		sdktrace.WithResource(res),
 		sdktrace.WithSampler(sampler),
 	)

internal/tracing/semconv.go

@@ -0,0 +1,65 @@
+// Package tracing provides OpenTelemetry OTLP trace export for the MCP Gateway.
+// This file is the single source of truth for go.opentelemetry.io/otel/semconv/v1.41.0
+// imports in the tracing package. All callers inside and outside this package should
+// reference these re-exports so that upgrading semconv only requires editing this file.
+package tracing
+
+import (
+	"go.opentelemetry.io/otel/attribute"
+	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
+)
+
+// SchemaURL is the semconv schema URL for this semantic conventions version.
+const SchemaURL = semconv.SchemaURL
+
+// HTTP and URL semantic convention attribute keys.
+const (
+	// HTTPRequestMethodKey is the HTTP request method.
+	HTTPRequestMethodKey = semconv.HTTPRequestMethodKey
+	// HTTPRouteKey is the matched HTTP route template.
+	HTTPRouteKey = semconv.HTTPRouteKey
+	// HTTPResponseStatusCodeKey is the HTTP response status code.
+	HTTPResponseStatusCodeKey = semconv.HTTPResponseStatusCodeKey
+	// URLPathKey is the full URL path.
+	URLPathKey = semconv.URLPathKey
+	// ServerAddressKey is the server domain name or IP address.
+	ServerAddressKey = semconv.ServerAddressKey
+)
+
+// Error semantic convention keys.
+const (
+	// ErrorTypeKey is the attribute key for error.type.
+	ErrorTypeKey = semconv.ErrorTypeKey
+)
+
+// GenAI semantic convention attribute keys (semconv/v1.41.0).
+// gen_ai.system was removed from the spec; see GenAISystem in genai_attrs.go.
+const (
+	// GenAIToolName is the name of the tool utilized by the agent.
+	GenAIToolName = semconv.GenAIToolNameKey
+	// GenAIOperationName is the name of the operation being performed.
+	GenAIOperationName = semconv.GenAIOperationNameKey
+	// GenAIConversationID is the unique identifier for a conversation (session).
+	GenAIConversationID = semconv.GenAIConversationIDKey
+	// GenAIAgentName is the human-readable name of the GenAI agent.
+	GenAIAgentName = semconv.GenAIAgentNameKey
+	// GenAIAgentID is the unique identifier of the GenAI agent (server ID).
+	GenAIAgentID = semconv.GenAIAgentIDKey
+)
+
+// ErrorType returns the error.type attribute KeyValue for err.
+// It wraps semconv.ErrorType so callers outside this package can use it
+// without importing the semconv package directly.
+func ErrorType(err error) attribute.KeyValue {
+	return semconv.ErrorType(err)
+}
+
+// ServiceName returns the service.name attribute KeyValue for val.
+func ServiceName(val string) attribute.KeyValue {
+	return semconv.ServiceName(val)
+}
+
+// ServiceVersion returns the service.version attribute KeyValue for val.
+func ServiceVersion(val string) attribute.KeyValue {
+	return semconv.ServiceVersion(val)
+}