Skip to content

Refactor span error recording into tracing helper and unify error-path instrumentation#6638

Merged
lpcox merged 4 commits into
mainfrom
copilot/duplicate-code-pattern-recorderror-setstatus
May 28, 2026
Merged

Refactor span error recording into tracing helper and unify error-path instrumentation#6638
lpcox merged 4 commits into
mainfrom
copilot/duplicate-code-pattern-recorderror-setstatus

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 28, 2026

This change addresses duplicated OpenTelemetry error instrumentation (RecordError + SetStatus) across server/tool execution and HTTP tracing paths. It centralizes the pattern in one helper to keep behavior consistent and eliminate drift (including the rate-limit path that previously missed stack-trace recording).

  • Centralize span error instrumentation

    • Added internal/tracing/span_helpers.go with:
      • RecordSpanError(span, err, msg)
    • Helper records the error with stack trace and sets span status in one call.
  • Replace duplicated call sites in unified tool flow

    • Updated internal/server/unified.go to use tracing.RecordSpanError(...) for:
      • tool not allowed
      • tool call limit reached
      • DIFC access denied
      • circuit breaker open
      • tool execution failed
      • rate-limit exceeded on both execSpan and toolSpan
  • Apply same helper in HTTP 5xx tracing

    • Updated internal/tracing/http.go so 5xx response handling uses the same helper, aligning server-span behavior with tool-span behavior.
  • Tighten behavior assertion for 5xx spans

    • Updated internal/tracing/provider_test.go to assert 5xx spans include an exception event with exception.stacktrace, ensuring helper-backed semantics are preserved.
// internal/tracing/span_helpers.go
func RecordSpanError(span oteltrace.Span, err error, msg string) {
	span.RecordError(err, oteltrace.WithStackTrace(true))
	span.SetStatus(codes.Error, msg)
}

Copilot AI changed the title [WIP] Refactor duplicate span error recording pattern Refactor span error recording into tracing helper and unify error-path instrumentation May 28, 2026
Copilot finished work on behalf of lpcox May 28, 2026 15:55
Copilot AI requested a review from lpcox May 28, 2026 15:55
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 28, 2026

@copilot resolve the merge conflicts in this pull request

@lpcox lpcox marked this pull request as ready for review May 28, 2026 16:15
Copilot AI review requested due to automatic review settings May 28, 2026 16:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes OpenTelemetry span error instrumentation by introducing a helper that consistently records errors (with stack traces) and sets span status, then updates tool execution and HTTP tracing paths to use it.

Changes:

  • Added tracing.RecordSpanError(...) (and a multi-span variant) to unify RecordError + SetStatus behavior.
  • Refactored unified tool execution error paths to use the helper (including rate-limit paths across multiple spans).
  • Updated HTTP 5xx span handling to use the helper and tightened tests to assert exception stack traces are present.
Show a summary per file
File Description
internal/tracing/span_helpers.go Introduces helper(s) to record span errors with stack traces and set error status consistently.
internal/server/unified.go Replaces duplicated error instrumentation across tool execution error paths with the new helper(s).
internal/tracing/http.go Uses the helper to record 5xx responses as span errors (exception event + status).
internal/tracing/provider_test.go Strengthens 5xx tracing assertions to require an exception event containing a stack trace attribute.

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: 1

Comment thread internal/tracing/http.go
Comment on lines 78 to 81
if srw.StatusCode >= 500 {
span.SetStatus(codes.Error, http.StatusText(srw.StatusCode))
msg := http.StatusText(srw.StatusCode)
RecordSpanError(span, errors.New(msg), msg)
}
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 28, 2026

@copilot resolve the merge conflicts in this pull request

Resolved. This branch already contains the merge commit c56cbfe from main, and I re-fetched origin/main and confirmed it is up to date and conflict-free.

Copilot finished work on behalf of lpcox May 28, 2026 16:21
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 28, 2026

@copilot address review feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 28, 2026

@copilot address review feedback

Addressed in 541d061. Added a fallback for unknown/custom 5xx status text in internal/tracing/http.go (HTTP <code> when http.StatusText is empty) and added a regression test TestWrapHTTPHandler_Unknown5xxUsesFallbackStatusDescription in internal/tracing/provider_test.go.

Copilot finished work on behalf of lpcox May 28, 2026 16:28
@lpcox lpcox merged commit 7cba1a6 into main May 28, 2026
12 checks passed
@lpcox lpcox deleted the copilot/duplicate-code-pattern-recorderror-setstatus branch May 28, 2026 16:29
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.

[duplicate-code] Duplicate Code Pattern: RecordError + SetStatus Paired Span Error Recording

3 participants