Skip to content

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

Merged
lpcox merged 3 commits into
mainfrom
copilot/go-fan-module-review
Jun 15, 2026
Merged

Fix missing OTel span error recording in proxy handler; consolidate to semconv/v1.41.0#7576
lpcox merged 3 commits into
mainfrom
copilot/go-fan-module-review

Conversation

Copilot AI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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)
// 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.

GitHub Advanced Security started work on behalf of lpcox June 15, 2026 13:43 View session
GitHub Advanced Security finished work on behalf of lpcox June 15, 2026 13:45
…semconv import in genai_attrs.go

- P0 Bug: Add tracing.RecordSpanError/RecordSpanErrorOnAll calls for the three
  error paths in handleWithDIFC that ended spans without marking them as errors:
  1. Guard not initialized (returns HTTP 503)
  2. RunPipelinePrePhases non-access-denied failure (returns HTTP 502)
  3. forwardAndReadBody returns nil/network error (returns HTTP 502)

- P1 Cleanup: Consolidate genai_attrs.go from semconv/v1.34.0 to semconv/v1.41.0
  for the five GenAI attribute keys available in v1.41.0 (GenAIToolNameKey,
  GenAIOperationNameKey, GenAIConversationIDKey, GenAIAgentNameKey, GenAIAgentIDKey).
  GenAISystem is defined as a raw attribute.Key since gen_ai.system was removed
  from semconv/v1.41.0, preserving the same string value for backward compatibility.

Closes #7569
Copilot AI changed the title [WIP] Review OpenTelemetry Go SDK usage in go-aw Fix missing OTel span error recording in proxy handler; consolidate to semconv/v1.41.0 Jun 15, 2026
Copilot AI requested a review from lpcox June 15, 2026 13:51
Copilot finished work on behalf of lpcox June 15, 2026 13:51
GitHub Advanced Security started work on behalf of lpcox June 15, 2026 14:01 View session
GitHub Advanced Security finished work on behalf of lpcox June 15, 2026 14:02
@lpcox lpcox marked this pull request as ready for review June 15, 2026 14:34
Copilot AI review requested due to automatic review settings June 15, 2026 14:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request improves OpenTelemetry observability by ensuring proxy DIFC pipeline failures are correctly recorded as span errors (so 502/503 failures don’t appear as successful traces), and it standardizes semantic convention usage by consolidating GenAI attribute keys on semconv/v1.41.0.

Changes:

  • Record span errors on three early-return failure paths in proxyHandler.handleWithDIFC (including marking both forward + pipeline spans when upstream forwarding fails).
  • Switch internal/tracing/genai_attrs.go to semconv/v1.41.0 and preserve the legacy gen_ai.system key via a raw attribute.Key.
Show a summary per file
File Description
internal/tracing/genai_attrs.go Updates GenAI semconv import to v1.41.0 and preserves gen_ai.system as a raw key.
internal/proxy/handler.go Records span errors for previously unmarked DIFC/forwarding failure paths.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 3

Comment thread internal/tracing/genai_attrs.go Outdated
Comment on lines +11 to +13
// 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.
// (semconv/v1.41.0), re-exported here for convenience.
Comment thread internal/proxy/handler.go
Comment on lines 186 to 190
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"), "service unavailable")
httputil.WriteErrorResponse(w, http.StatusServiceUnavailable, "service_unavailable", "proxy enforcement not configured")
Comment thread internal/proxy/handler.go
Comment on lines 235 to 238
}
if resp == nil {
tracing.RecordSpanErrorOnAll(errors.New("upstream request failed"), "upstream request failed", fwdSpan, difcSpan)
return
@lpcox

lpcox commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Copilot AI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address review feedback

All three review items addressed in the latest commit:

  1. genai_attrs.go comment — updated the block comment to note that most keys are semconv aliases except GenAISystem, which is special-cased as a raw attribute.Key for wire compatibility.

  2. Span status description — changed "service unavailable" to "proxy enforcement not configured" to match the client-facing message returned on that path (consistent with the other DIFC error paths).

  3. Span error tests — added three new tests in handler_difc_test.go that use an injected in-memory recording tracer via CachedTracer to assert span status and exception events on each failure path:

    • TestHandleWithDIFC_GuardNotInitialized_SetsSpanError (503)
    • TestHandleWithDIFC_LabelResourceError_SetsSpanError (502 — pre-phases failure)
    • TestHandleWithDIFC_UpstreamFailure_SetsSpanError (502 — both fwdSpan and difcSpan marked Error)

GitHub Advanced Security started work on behalf of lpcox June 15, 2026 15:33 View session
Copilot finished work on behalf of lpcox June 15, 2026 15:33
GitHub Advanced Security finished work on behalf of lpcox June 15, 2026 15:35
@lpcox

lpcox commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Copilot AI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address review feedback

All review feedback was already addressed in commit 861ffa2. Verified make agent-finished passes cleanly (format → build → lint → Go tests → Rust guard tests).

To summarise what's in that commit:

  • genai_attrs.go comment — block comment now explicitly notes that GenAISystem is special-cased as a raw attribute.Key (not a semconv alias) for wire compatibility.
  • Span status description"service unavailable" replaced with "proxy enforcement not configured" to match the client-facing message and be consistent with the other DIFC error paths.
  • Span error tests — three tests added in handler_difc_test.go using an injected in-memory recording tracer (tracetest.NewInMemoryExporter) via CachedTracer: TestHandleWithDIFC_GuardNotInitialized_SetsSpanError, TestHandleWithDIFC_LabelResourceError_SetsSpanError, and TestHandleWithDIFC_UpstreamFailure_SetsSpanError.

Copilot finished work on behalf of lpcox June 15, 2026 16:44
@lpcox lpcox merged commit b67ede9 into main Jun 15, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants