Conversation
4 tasks
…ump semconv to v1.27.0 Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/111ffb27-9d69-41be-b824-8c1882bf7279 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
… attribute setting Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/111ffb27-9d69-41be-b824-8c1882bf7279 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Review OpenTelemetry Go module go.opentelemetry.io/otel
feat(tracing): HTTP response status on spans, sanitize SetStatus description, bump semconv to v1.27.0
Apr 30, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens OpenTelemetry tracing behavior in the gateway/proxy by recording HTTP response status codes on server spans, reducing sensitive error text exposure via span status descriptions, and updating the semconv import version used across the codebase.
Changes:
- Add an
http.ResponseWriterwrapper inWrapHTTPHandlerto capture and attachhttp.response.status_code, and setcodes.Errorfor 5xx responses. - Replace
SetStatus(..., err.Error())with a generic status description for backend tool execution spans. - Bump semconv imports from
v1.26.0tov1.27.0in the affected files.
Show a summary per file
| File | Description |
|---|---|
| internal/tracing/http.go | Wraps ResponseWriter to capture/set HTTP response status code on spans and mark 5xx as errors. |
| internal/tracing/provider.go | Updates semconv import used for resource schema URL and service attributes. |
| internal/server/unified.go | Sanitizes span status description for backend tool execution errors; updates semconv import. |
| internal/proxy/handler.go | Updates semconv import used for span attributes in proxy request tracing. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
internal/tracing/http.go:25
- WriteHeader currently overwrites rw.statusCode on every call, but net/http only sends the first status code (subsequent WriteHeader calls are ignored). This can cause the span to record a status code that was never actually written to the client. Track whether headers have already been written (or only set statusCode on the first call) so the recorded status matches the real response.
func (rw *statusResponseWriter) WriteHeader(code int) {
rw.statusCode = code
rw.ResponseWriter.WriteHeader(code)
}
internal/tracing/http.go:33
- statusResponseWriter is initialized with statusCode=http.StatusOK, so the rw.statusCode==0 check in Write is dead code and the comment about “implicit 200 when Write is called without WriteHeader” is misleading. Either initialize statusCode to 0 and set it on the first Write/WriteHeader (and treat 0 as “not written yet”), or remove the conditional/comment to keep the wrapper’s behavior and documentation consistent.
// Write captures an implicit 200 status when Write is called without a prior WriteHeader.
func (rw *statusResponseWriter) Write(b []byte) (int, error) {
if rw.statusCode == 0 {
rw.statusCode = http.StatusOK
}
return rw.ResponseWriter.Write(b)
}
- Files reviewed: 4/4 changed files
- Comments generated: 4
Comment on lines
+16
to
+20
| // statusResponseWriter wraps http.ResponseWriter to capture the HTTP response status code. | ||
| type statusResponseWriter struct { | ||
| http.ResponseWriter | ||
| statusCode int | ||
| } |
Comment on lines
+71
to
+76
| srw := &statusResponseWriter{ResponseWriter: w, statusCode: http.StatusOK} | ||
| defer func() { | ||
| span.SetAttributes(semconv.HTTPResponseStatusCodeKey.Int(srw.statusCode)) | ||
| if srw.statusCode >= 500 { | ||
| span.SetStatus(codes.Error, http.StatusText(srw.statusCode)) | ||
| } |
| "go.opentelemetry.io/otel/sdk/resource" | ||
| sdktrace "go.opentelemetry.io/otel/sdk/trace" | ||
| semconv "go.opentelemetry.io/otel/semconv/v1.26.0" | ||
| semconv "go.opentelemetry.io/otel/semconv/v1.27.0" |
Comment on lines
555
to
558
| execSpan.RecordError(err) | ||
| execSpan.SetStatus(codes.Error, err.Error()) | ||
| execSpan.SetStatus(codes.Error, "tool execution failed") | ||
| httpStatusCode = 500 | ||
| return mcp.NewErrorCallToolResult(err) |
Collaborator
|
@copilot address the review feedback |
…r, add status tests Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/846b06ec-3baf-4a96-879e-dc60abf3081d Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Contributor
Author
Addressed all four review points in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
feat(tracing): HTTP response status on spans, sanitize SetStatus description, bump semconv to v1.27.0
feat(tracing): HTTP response status on spans, sanitize error recording, bump semconv to v1.27.0
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three OTel best-practice gaps identified in the Go Fan module review:
WrapHTTPHandlernever recorded the HTTP response status code,SetStatusandRecordErrorwere leaking raw error strings to trace backends, and all files were pinned to semconvv1.26.0despitev1.27.0being available.Changes
tracing/http.go— capture HTTP response status on spansstatusResponseWriterwrapper interceptingWriteHeaderandWrite(handles implicit 200 when onlyWriteis called)Unwrap() http.ResponseWriterto preserve optional interfaces (http.Flusher,http.Hijacker, etc.) so streaming/SSE handlers are not brokenhttp.response.status_codeattribute andcodes.Errorspan status for 5xx viadefer, ensuring attributes are recorded even if the handler panicsserver/unified.go— sanitize error recordingexecSpan.SetStatus(codes.Error, err.Error())→execSpan.SetStatus(codes.Error, "tool execution failed")execSpan.RecordError(err)→execSpan.RecordError(fmt.Errorf("tool execution failed"))— prevents raw upstream error text from appearing asexception.messagein trace backends; the full error is still returned to the caller viamcp.NewErrorCallToolResult(err)semconv
v1.26.0→v1.27.0across all four affected files (tracing/http.go,tracing/provider.go,server/unified.go,proxy/handler.go); no go.mod/go.sum changes required as semconv is a sub-package of the already-importedgo.opentelemetry.io/otel v1.43.0Tests — added three new test cases to
internal/tracing/provider_test.gocovering the HTTP status recording behaviour:WriteHeader(404)is reflected in the span attribute; 4xx does not setcodes.ErrorWritewithoutWriteHeaderis correctly recordedcodes.Errorwith a non-empty description