Fix missing OTel span error recording in proxy handler; consolidate to semconv/v1.41.0 (#7576)

Three error paths in `handleWithDIFC` silently ended spans without
marking them as errors, causing 502/503 failures to appear as successful
spans in tracing backends. Additionally, `genai_attrs.go` imported
`semconv/v1.34.0` solely for GenAI attribute keys while the rest of the
codebase uses `semconv/v1.41.0`.

## P0 — Span error recording gaps in `proxy/handler.go`

Three paths in `handleWithDIFC` now call `tracing.RecordSpanError` /
`RecordSpanErrorOnAll` before returning:

| Path | Status | Fix |
|---|---|---|
| Guard not initialized | 503 | `RecordSpanError(difcSpan, ...)` with
description `"proxy enforcement not configured"` |
| `RunPipelinePrePhases` non-access-denied failure | 502 |
`RecordSpanError(difcSpan, err, ...)` |
| `forwardAndReadBody` returns nil | 502 | `RecordSpanErrorOnAll(...,
fwdSpan, difcSpan)` |

```go
// Case 3: both child and parent spans now reflect the failure
if resp == nil {
    tracing.RecordSpanErrorOnAll(errors.New("upstream request failed"), "upstream request failed", fwdSpan, difcSpan)
    return
}
```

## P1 — Consolidate dual semconv imports

Switches `genai_attrs.go` from `semconv/v1.34.0` to `semconv/v1.41.0` —
all five needed GenAI keys (`GenAIToolNameKey`, `GenAIOperationNameKey`,
`GenAIConversationIDKey`, `GenAIAgentNameKey`, `GenAIAgentIDKey`) are
present in v1.41.0. `GenAISystem` is defined as a raw
`attribute.Key("gen_ai.system")` since that constant was dropped from
v1.41.0, preserving the same wire string. The block comment on these
constants is updated to make clear that `GenAISystem` is special-cased
and not a semconv alias.

## P2 — Regression tests for span error recording

Added `internal/proxy/handler_difc_test.go` with three tests that inject
an in-memory recording tracer via `CachedTracer` and assert span status
code and exception events on each failure path:

- `TestHandleWithDIFC_GuardNotInitialized_SetsSpanError` — verifies the
503 path sets `Error` status with description `"proxy enforcement not
configured"` and emits an `exception` event on the DIFC pipeline span.
- `TestHandleWithDIFC_LabelResourceError_SetsSpanError` — verifies the
502 pre-phases-failure path sets `Error` status with description
`"resource labeling failed"` and emits an `exception` event.
- `TestHandleWithDIFC_UpstreamFailure_SetsSpanError` — verifies the 502
upstream-nil path sets `Error` status on both the
`proxy.backend.forward` child span and the `proxy.difc_pipeline` parent
span.

Author: lpcox

internal/proxy/handler.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -185,6 +186,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 		errMsg := "returning 503: proxy enforcement not configured (no --policy flag provided)"
 		logHandler.Print(errMsg)
 		logger.LogError("proxy", "%s", errMsg)
+		tracing.RecordSpanError(difcSpan, errors.New("proxy enforcement not configured"), "proxy enforcement not configured")
 		httputil.WriteErrorResponse(w, http.StatusServiceUnavailable, "service_unavailable", "proxy enforcement not configured")
 		return
 	}
@@ -211,6 +213,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 			return
 		}
 		logHandler.Printf("[DIFC] Phase 1 failed: %v", err)
+		tracing.RecordSpanError(difcSpan, err, "resource labeling failed")
 		httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "resource labeling failed")
 		return
 	}
@@ -231,6 +234,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 		fwdSpan.SetAttributes(semconv.HTTPResponseStatusCodeKey.Int(resp.StatusCode))
 	}
 	if resp == nil {
+		tracing.RecordSpanErrorOnAll(errors.New("upstream request failed"), "upstream request failed", fwdSpan, difcSpan)
 		return
 	}
 

internal/proxy/handler_difc_test.go

@@ -11,8 +11,11 @@ import (
 
 	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/guard"
+	"github.com/github/gh-aw-mcpg/internal/tracing"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 )
 
 // stubGuard is a configurable test double for guard.Guard.
@@ -536,3 +539,123 @@ func tagsAsStrings(tags []difc.Tag) []string {
 	}
 	return result
 }
+
+// newRecordingProxyHandler creates a proxyHandler with an in-memory recording tracer
+// injected. The returned function flushes and returns all recorded spans.
+func newRecordingProxyHandler(t *testing.T, s *Server) (*proxyHandler, func() []tracetest.SpanStub) {
+	t.Helper()
+	exporter := tracetest.NewInMemoryExporter()
+	sp := sdktrace.NewSimpleSpanProcessor(exporter)
+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithSpanProcessor(sp),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
+	)
+	t.Cleanup(func() { _ = tp.Shutdown(t.Context()) })
+	h := &proxyHandler{
+		server:       s,
+		CachedTracer: tracing.CachedTracer{Tracer: tp.Tracer("test")},
+	}
+	return h, func() []tracetest.SpanStub { return exporter.GetSpans() }
+}
+
+// spanByName returns the first recorded span with the given name, or nil.
+func spanByName(spans []tracetest.SpanStub, name string) *tracetest.SpanStub {
+	for i := range spans {
+		if spans[i].Name == name {
+			return &spans[i]
+		}
+	}
+	return nil
+}
+
+// ─── Span error recording: guard not initialized → 503 ───────────────────────
+
+func TestHandleWithDIFC_GuardNotInitialized_SetsSpanError(t *testing.T) {
+	s := &Server{
+		guard: guard.NewNoopGuard(),
+		DIFCComponents: difc.DIFCComponents{
+			Mode:          difc.EnforcementFilter,
+			Evaluator:     difc.NewEvaluatorWithMode(difc.EnforcementFilter),
+			AgentRegistry: difc.NewAgentRegistryWithDefaults(nil, nil),
+			Capabilities:  difc.NewCapabilities(),
+		},
+		githubAPIURL:     "http://unused",
+		httpClient:       &http.Client{},
+		guardInitialized: false,
+	}
+	h, getSpans := newRecordingProxyHandler(t, s)
+
+	req := httptest.NewRequest(http.MethodGet, "/repos/org/repo/issues", nil)
+	w := httptest.NewRecorder()
+	h.handleWithDIFC(w, req, "/repos/org/repo/issues", "list_issues",
+		map[string]interface{}{}, nil)
+
+	assert.Equal(t, http.StatusServiceUnavailable, w.Code)
+
+	spans := getSpans()
+	difcSpan := spanByName(spans, "proxy.difc_pipeline")
+	require.NotNil(t, difcSpan, "proxy.difc_pipeline span must be recorded")
+	assert.Equal(t, "Error", difcSpan.Status.Code.String(), "difc span status must be Error")
+	assert.Equal(t, "proxy enforcement not configured", difcSpan.Status.Description)
+	require.NotEmpty(t, difcSpan.Events, "difc span must have exception event")
+	assert.Equal(t, "exception", difcSpan.Events[0].Name)
+}
+
+// ─── Span error recording: RunPipelinePrePhases failure → 502 ────────────────
+
+func TestHandleWithDIFC_LabelResourceError_SetsSpanError(t *testing.T) {
+	upstream := mockUpstream(t, http.StatusOK, []interface{}{map[string]interface{}{"id": 1}})
+	defer upstream.Close()
+
+	g := &stubGuard{
+		labelResourceErr: errors.New("guard unavailable"),
+	}
+	s := newTestServerWithStub(t, upstream.URL, g, difc.EnforcementFilter)
+	h, getSpans := newRecordingProxyHandler(t, s)
+
+	req := httptest.NewRequest(http.MethodGet, "/repos/org/repo/issues", nil)
+	w := httptest.NewRecorder()
+	h.handleWithDIFC(w, req, "/repos/org/repo/issues", "list_issues",
+		map[string]interface{}{"owner": "org", "repo": "repo"}, nil)
+
+	assert.Equal(t, http.StatusBadGateway, w.Code)
+
+	spans := getSpans()
+	difcSpan := spanByName(spans, "proxy.difc_pipeline")
+	require.NotNil(t, difcSpan, "proxy.difc_pipeline span must be recorded")
+	assert.Equal(t, "Error", difcSpan.Status.Code.String(), "difc span status must be Error")
+	assert.Equal(t, "resource labeling failed", difcSpan.Status.Description)
+	require.NotEmpty(t, difcSpan.Events, "difc span must have exception event")
+	assert.Equal(t, "exception", difcSpan.Events[0].Name)
+}
+
+// ─── Span error recording: upstream forwarding failure → 502 ─────────────────
+
+func TestHandleWithDIFC_UpstreamFailure_SetsSpanError(t *testing.T) {
+	// Point at a port that refuses connections so forwardAndReadBody returns nil.
+	g := &stubGuard{
+		labelResourceResult: publicResource(),
+		labelResourceOp:     difc.OperationRead,
+	}
+	s := newTestServerWithStub(t, "http://127.0.0.1:1", g, difc.EnforcementFilter)
+	h, getSpans := newRecordingProxyHandler(t, s)
+
+	req := httptest.NewRequest(http.MethodGet, "/repos/org/repo/issues", nil)
+	w := httptest.NewRecorder()
+	h.handleWithDIFC(w, req, "/repos/org/repo/issues", "list_issues",
+		map[string]interface{}{"owner": "org", "repo": "repo"}, nil)
+
+	assert.Equal(t, http.StatusBadGateway, w.Code)
+
+	spans := getSpans()
+
+	difcSpan := spanByName(spans, "proxy.difc_pipeline")
+	require.NotNil(t, difcSpan, "proxy.difc_pipeline span must be recorded")
+	assert.Equal(t, "Error", difcSpan.Status.Code.String(), "difc span status must be Error")
+	assert.Equal(t, "upstream request failed", difcSpan.Status.Description)
+
+	fwdSpan := spanByName(spans, "proxy.backend.forward")
+	require.NotNil(t, fwdSpan, "proxy.backend.forward span must be recorded")
+	assert.Equal(t, "Error", fwdSpan.Status.Code.String(), "fwd span status must be Error")
+	assert.Equal(t, "upstream request failed", fwdSpan.Status.Description)
+}

internal/tracing/genai_attrs.go

@@ -5,15 +5,18 @@ package tracing
 
 import (
 	"go.opentelemetry.io/otel/attribute"
-	semconv "go.opentelemetry.io/otel/semconv/v1.34.0"
+	semconv "go.opentelemetry.io/otel/semconv/v1.41.0"
 )
 
 // GenAI semantic convention attribute keys.
-// These are aliases for the official OpenTelemetry gen_ai semconv constants
-// (semconv/v1.34.0), re-exported here for convenience.
+// 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.
-	GenAISystem = semconv.GenAISystemKey
+	// 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