Skip to content

fix: Stop reporting response bytes that were never written - #792

Merged
keelerm84 merged 1 commit into
v9from
mk/SDK-2842/response-write-span
Aug 3, 2026
Merged

fix: Stop reporting response bytes that were never written#792
keelerm84 merged 1 commit into
v9from
mk/SDK-2842/response-write-span

Conversation

@keelerm84

@keelerm84 keelerm84 commented Aug 3, 2026

Copy link
Copy Markdown
Member

Summary

relay.response.bytes on the relay.response.write span was set before the write, from len(payload), in all five polling handlers, and the write's result was discarded. It reported the payload size under the name of the response.

Measured against a real relay, GET /sdk/flags, 3310-byte payload:

Scenario relay.payload.bytes (serialize span) relay.response.bytes (write span) http.response.body.size (request span) actual wire body
200, no compression 3310 3310 3310 3310
200, compression on 3310 3310 532 532
304 (If-None-Match hit) 3310 3310 absent 0

The 304 row is the ordinary SDK polling path. A short write or client disconnect was the third divergence: the partial count never appeared and the span status stayed OK, so the span named relay.response.write could not tell a complete response from a truncated one.

Taking the count from w.Write would not have fixed the compressed case. The handler's ResponseWriter is the gzip writer -- the compression middleware is registered outside the handler -- and gzhttp.Write returns input bytes consumed, not compressed output. The truthful count already exists on the request span as http.response.body.size, recorded by the HTTP tracing middleware from a counter outside the compression middleware, which is the only place wire bytes are observable; it is correct in every row above, and correctly absent rather than zero on the 304.

So this drops relay.response.bytes and tracing.ResponseBytesKey. The division of labour is now:

  • serialize span -- relay.payload.bytes: what was built.
  • request span -- http.response.body.size: what actually went out, after compression.
  • write span -- duration, plus two things it was missing:
    • http.response.status_code, so a body-less 304 explains itself instead of looking like a lost payload.
    • RecordError and an error span status when the write fails. This is the load-bearing part: a write that fails once the header is out cannot change the status code, so the request span's status stays unset and the write span is the only place that failure can surface.

The five near-identical write-span closures collapse into one traceWriteResponse helper, which also removes the dupl lint exposure flagged during the review of #784. writeCacheableJSONResponse now returns (status, error).

Tests: the previous payloadBytes == responseBytes assertion was a tautology -- both sides were len() of the same slice -- and goes away with the attribute it compared. New relay_endpoints_write_span_test.go covers the three divergences directly: the 304 path, the compressed path, and a write that fails after the header is out. Mutation-checked -- dropping RecordError, returning 200 on the etag path, or reporting a wrong status code each fail tests.

Also documented in the helper, since it is the same confusion one level down: the span measures the time to hand the payload to the ResponseWriter, not time on the socket -- a response small enough for net/http's output buffer is flushed after the handler returns.

Found by a multi-agent review of #784 (finding #3), which introduced these spans.

Independent of #791 (also open, also off v9): no overlapping files, not stacked.


Note

Low Risk
Telemetry-only changes to polling handlers with added tests; no auth, data, or API behavior changes beyond more accurate tracing.

Overview
Fixes misleading relay.response.bytes on relay.response.write spans, which were set from len(payload) before the write and could disagree with what actually went on the wire (gzip, 304, truncated responses).

Removes tracing.ResponseBytesKey and stops attributing byte counts on the write span. Payload size stays on the serialize span (relay.payload.bytes); wire size is left to the request span (http.response.body.size from HTTP middleware, outside compression).

Polling handlers now share traceWriteResponse, which records http.response.status_code on the write span and RecordError / error status when Write fails after headers are sent—cases the request span cannot reflect. writeCacheableJSONResponse returns (status, error) so failures are observable.

Tests drop the tautological payload-vs-response bytes check and add coverage for 304, compression, and post-header write failures.

Reviewed by Cursor Bugbot for commit c99115c. Bugbot is set up for automated code reviews on this repo. Configure here.

relay.response.bytes was set before the write, from len(payload), in all
five polling handlers, and the write's result was discarded. It reported
the payload size under the name of the response, and diverged from what
was actually sent in three ways: on a 304 (the ordinary SDK polling path)
it reported the full payload though no body was written; with compression
enabled it reported the pre-compression size; and on a short write or
client disconnect it reported the whole payload while the span status
stayed OK, so the span could not tell a complete response from a
truncated one.

Taking the count from w.Write would not have fixed the compressed case:
the handler's ResponseWriter is the gzip writer, and gzhttp.Write returns
input bytes consumed. The truthful count already exists on the request
span as http.response.body.size, recorded outside the compression
middleware, which is the only place wire bytes are observable.

So drop relay.response.bytes. What was built is on the serialize span as
relay.payload.bytes, and what went out is on the request span. The write
span now records the status code it sent, so a body-less 304 explains
itself, and records the write error when one occurs -- a write that fails
after the header is out cannot change the status code, so this span is
the only place that failure surfaces.

The five near-identical write-span closures collapse into one
traceWriteResponse helper.
@keelerm84
keelerm84 requested a review from kinyoklion August 3, 2026 17:22
@keelerm84
keelerm84 marked this pull request as ready for review August 3, 2026 17:23
@keelerm84
keelerm84 requested a review from a team as a code owner August 3, 2026 17:23
@keelerm84
keelerm84 merged commit 4d6c758 into v9 Aug 3, 2026
18 checks passed
@keelerm84
keelerm84 deleted the mk/SDK-2842/response-write-span branch August 3, 2026 19:51
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.

2 participants