fix problems when writing to telemetry

[yrobla]

Added a field to track if headers had been written already and prevent multiple calls. Add flush for managing response flushing when needed.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.69%. Comparing base (44aa334) to head (c9f8d5d).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/telemetry/middleware.go	83.33%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2127      +/-   ##
==========================================
+ Coverage   48.67%   48.69%   +0.02%     
==========================================
  Files         241      241              
  Lines       30576    30612      +36     
==========================================
+ Hits        14882    14906      +24     
- Misses      14574    14578       +4     
- Partials     1120     1128       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull Request Overview

This PR fixes issues with HTTP response handling in the telemetry middleware by preventing duplicate header writes and adding proper interface support. The changes ensure that HTTP response writers behave correctly when headers are written multiple times and add support for flushing and connection hijacking.

• Added header tracking to prevent multiple WriteHeader calls
• Implemented http.Flusher and http.Hijacker interfaces for better HTTP compatibility
• Enhanced response writer with proper state management

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[danbarr]

Tested and confirmed this fixes #2114

[JAORMX]

@claude please review this

[claude]

Claude finished @JAORMX's task —— View job

---

Code Review

Summary

This PR correctly addresses HTTP response writer issues in the telemetry middleware by preventing duplicate WriteHeader calls and adding support for http.Flusher and http.Hijacker interfaces. The changes are well-implemented and fix real issues that can occur with middleware wrappers.

Positive Changes

1. Duplicate WriteHeader Protection (lines 413-416): The guard clause properly prevents multiple WriteHeader calls, which would otherwise cause Go's HTTP server to panic with "superfluous response.WriteHeader call".

2. Explicit Header Writing in Write() (lines 424-426): Ensures headers are written before body data, maintaining proper HTTP semantics.

3. Interface Implementations: Adding Flush() and Hijack() methods ensures the wrapper doesn't break code that type-asserts for these common interfaces (e.g., SSE connections, WebSocket upgrades).

4. Field Initialization (line 135): Explicitly initializing wroteHeader: false is good practice for clarity.

Issues & Suggestions

1. Missing Logging on Duplicate WriteHeader (copilot suggestion is valid)

The existing review comment from copilot is correct - silently returning on duplicate WriteHeader calls can hide programming errors. Consider adding:

func (rw *responseWriter) WriteHeader(statusCode int) {
    if rw.wroteHeader {
        logger.Warnf("WriteHeader called multiple times: attempted status %d, already wrote status %d", 
            statusCode, rw.statusCode)
        return
    }
    rw.statusCode = statusCode
    rw.wroteHeader = true
    rw.ResponseWriter.WriteHeader(statusCode)
}

2. Test Coverage (15 lines missing)

The codecov report shows only 11.76% patch coverage. Consider adding unit tests for:

• Multiple WriteHeader calls scenario
• Write() being called before WriteHeader()
• Flush() with and without underlying http.Flusher support
• Hijack() with and without underlying http.Hijacker support

Example test structure:

func TestResponseWriter_WriteHeaderIdempotent(t *testing.T) {
    // Test that multiple WriteHeader calls don't panic
}

func TestResponseWriter_WriteCallsWriteHeader(t *testing.T) {
    // Test that Write() automatically calls WriteHeader(200)
}

func TestResponseWriter_Flush(t *testing.T) {
    // Test Flush with mock ResponseWriter that implements http.Flusher
}

func TestResponseWriter_Hijack(t *testing.T) {
    // Test Hijack with mock ResponseWriter that implements http.Hijacker
}

3. Error Message in Hijack (line 444)

The error message "http.Hijacker not supported" could be more descriptive:

return nil, nil, fmt.Errorf("underlying http.ResponseWriter does not implement http.Hijacker")

Minor Observations

• The wroteHeader field could have a doc comment explaining its purpose
• Consider whether the Hijack() method needs to update wroteHeader state (typically hijacking happens before headers are written, but worth considering)

Verdict

The core logic is correct and solves the stated problem. The main gaps are:

1. Missing logging for detection of programming errors
2. Insufficient test coverage

These are both addressable in follow-up work if needed. The PR is functional as-is and fixes issue #2114.

---

Job Run | Branch: issue-2114

[blkt]

question: sending an HTTP 200 is the default behavior when no calls to WriteHeader are issued before the first Write call. Specifically

If WriteHeader is not called explicitly, the first call to Write
will trigger an implicit WriteHeader(http.StatusOK).
Thus explicit calls to WriteHeader are mainly used to
send error codes or 1xx informational responses.

Additionally

1xx headers are sent immediately, but 2xx-5xx
headers may be buffered. Use the Flusher interface to send
buffered data.

I see you added the Flusher interface. 👍

Excluding the sending of 1xx status codes (which we don't send, IIRC), I don't understand how preventing WriteHeader from running twice fixes the issue. Could you explain me what was the root cause of the bug?

[blkt]

nit: I don't think this is necessary, is it?

[yrobla]

having some issues with pr. it's not picking latest code, not picking rebases...

[danbarr]

@yrobla Yes, GitHub was having a lot of issues today, but looks like everything has recovered