Skip to content

[Repo Assist] refactor: extract RecordSpanError helper to eliminate duplicate span error recording#6632

Merged
lpcox merged 1 commit into
mainfrom
repo-assist/improve-record-span-error-5dec1c9711102669
May 28, 2026
Merged

[Repo Assist] refactor: extract RecordSpanError helper to eliminate duplicate span error recording#6632
lpcox merged 1 commit into
mainfrom
repo-assist/improve-record-span-error-5dec1c9711102669

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This is an automated pull request from Repo Assist, an AI assistant.

Summary

Addresses the duplicate code pattern identified in #6616. The pattern:

span.RecordError(err, oteltrace.WithStackTrace(true))
span.SetStatus(codes.Error, msg)

was repeated 5 times in internal/server/unified.go. Additionally, the dual-span rate-limit block used 4 separate lines across execSpan and toolSpan.

Changes

New file: internal/tracing/span_helpers.go

  • RecordSpanError(span, err, msg) — records an error on a span with stack trace and sets status to Error
  • RecordSpanErrorOnAll(err, msg, spans...) — same, applied to multiple spans at once

Updated: internal/server/unified.go

  • Replaced all 5 duplicate 2-line patterns with tracing.RecordSpanError
  • Collapsed the 4-line dual-span rate-limit block into one tracing.RecordSpanErrorOnAll call
  • Removed the now-unused go.opentelemetry.io/otel/codes import

Bug fix included

The two RecordError calls at the rate-limit path (lines 525–526) were missing WithStackTrace(true) — an inconsistency noted in #6616. The new helper always enables stack traces, making all error paths consistent.

Trade-offs

  • The helper is minimal and doesn't abstract the response-writing concern — callers still decide what to return after recording the error
  • RecordSpanErrorOnAll uses a variadic span slice, keeping the common single-span case clean

Test Status

⚠️ Infrastructure limitation: The sandbox environment does not have network access to download Go module dependencies (proxy.golang.org is firewalled), so make agent-finished could not be run. The changes are a straightforward mechanical extraction — no logic change, only removing the codes import (verified it's unused after the refactor) and adding a new file with two small helpers.

Closes #6616

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by Repo Assist · sonnet46 3.8M ·

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@851905c06e905bf362a9f6cc54f912e3df747d55

…error recording

The pattern of calling span.RecordError(err, oteltrace.WithStackTrace(true))
immediately followed by span.SetStatus(codes.Error, msg) was repeated 5 times
in internal/server/unified.go and once more in internal/tracing/http.go.

This commit introduces two helpers in internal/tracing/span_helpers.go:
- RecordSpanError: single-span error recording with stack trace + status
- RecordSpanErrorOnAll: same, applied to multiple spans at once

All 5 duplicate occurrences in unified.go are replaced with RecordSpanError,
and the 4-line dual-span rate-limit block is collapsed into one
RecordSpanErrorOnAll call.

This also fixes a latent inconsistency: the two RecordError calls at the
rate-limit site were missing WithStackTrace(true), unlike all other error
paths. The new helper always enables stack traces.

Closes #6616

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review May 28, 2026 15:45
Copilot AI review requested due to automatic review settings May 28, 2026 15:45
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 recording for backend tool-call paths, reducing duplicated RecordError + SetStatus logic and making stack-trace recording consistent.

Changes:

  • Adds tracing.RecordSpanError and tracing.RecordSpanErrorOnAll.
  • Replaces duplicated span error-recording blocks in internal/server/unified.go.
  • Removes the now-unused codes import from unified.go.
Show a summary per file
File Description
internal/tracing/span_helpers.go Adds shared helpers for recording span errors with stack traces and error status.
internal/server/unified.go Uses the new tracing helpers across tool denial, DIFC denial, circuit breaker, execution failure, and rate-limit 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: 0

@lpcox lpcox merged commit 63c9f69 into main May 28, 2026
23 checks passed
@lpcox lpcox deleted the repo-assist/improve-record-span-error-5dec1c9711102669 branch May 28, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants