feat(router): add metric request timetolastbyte - #3119
Conversation
…-add-metric-request-timetolastbyte
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds time-to-last-request-byte and time-to-last-byte tracing, metrics, expression fields, tests, schemas, and documentation. Measurements coordinate asynchronous request callbacks with response-body completion and exclude unsupported response scenarios. ChangesLast-byte network observability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (84.74%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3119 +/- ##
==========================================
+ Coverage 62.37% 62.50% +0.13%
==========================================
Files 262 262
Lines 31003 31168 +165
==========================================
+ Hits 19337 19483 +146
- Misses 10158 10170 +12
- Partials 1508 1515 +7
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
router/internal/traceclient/traceclient_test.go (3)
811-811: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
require.Sameto assert the body is not wrapped.
require.Equalis a deep-value comparison; the invariant here is pointer identity, which Line 900 already expresses withrequire.Same.♻️ Assert identity
- require.Equal(t, tt.body, resp.Body) + require.Same(t, tt.body, resp.Body)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/traceclient/traceclient_test.go` at line 811, Update the body assertion in the relevant traceclient test to use require.Same instead of require.Equal, matching the identity assertion already used near line 900 and verifying that resp.Body is the exact same object as tt.body.
596-600: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAbsolute upper bound at Line 599 is timing-fragile.
Expected value is ~20ms against a 40ms ceiling;
time.Sleepguarantees only a lower bound, so a loaded runner can push two 10ms paced reads past 40ms. The semantics you care about are already covered by the lower bound (Line 598) and the ordering assertion (Line 600). Consider widening the server-wait sleep and the ceiling, or dropping the absolute ceiling in favor of the relative comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/traceclient/traceclient_test.go` around lines 596 - 600, Remove the timing-fragile absolute upper-bound assertion on results.TimeToLastByte in the test, relying on the existing lower-bound and TimeToFirstByte ordering assertions to validate the intended semantics. Keep the ttlb count and remaining timing checks unchanged.
107-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pacedBody.Readsilently drops chunk remainders on short buffers.
copyis bounded bylen(p), but the chunk index advances unconditionally. Current call sites all read with buffers at least as large as each chunk, so no assertion is wrong today, but a future test reading in small increments would silently lose bytes and skew last-byte assertions.♻️ Track the offset inside the current chunk
type pacedBody struct { chunks [][]byte delay time.Duration i int + off int closed bool } func (b *pacedBody) Read(p []byte) (int, error) { if b.i >= len(b.chunks) { return 0, io.EOF } time.Sleep(b.delay) - n := copy(p, b.chunks[b.i]) - b.i++ + n := copy(p, b.chunks[b.i][b.off:]) + b.off += n + if b.off >= len(b.chunks[b.i]) { + b.i++ + b.off = 0 + } return n, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/traceclient/traceclient_test.go` around lines 107 - 115, Update pacedBody.Read to retain an offset into the current chunk instead of advancing b.i after every read. Copy only the remaining chunk bytes that fit in p, advance the offset by n, and move to the next chunk only when the current chunk is fully consumed, preserving io.EOF behavior after all chunks are exhausted.router/internal/traceclient/traceclient.go (1)
422-441: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
errors.Is(err, io.EOF)for the completion check.A body wrapper further down the chain can return an EOF wrapped in another error, in which case the
==comparison misses completion and the metric is silently dropped.♻️ Proposed change
complete := false switch { - case err == io.EOF: + case errors.Is(err, io.EOF): // A short EOF on a response with an explicit Content-Length is a // truncated response, not a successfully observed last byte. complete = b.contentLength < 0 || b.bytesRead >= b.contentLengthAdd
"errors"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/traceclient/traceclient.go` around lines 422 - 441, Update timedResponseBody.Read to detect wrapped EOF errors with errors.Is(err, io.EOF) instead of direct equality, adding the errors import while preserving the existing content-length completion logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@router/internal/traceclient/traceclient_test.go`:
- Line 811: Update the body assertion in the relevant traceclient test to use
require.Same instead of require.Equal, matching the identity assertion already
used near line 900 and verifying that resp.Body is the exact same object as
tt.body.
- Around line 596-600: Remove the timing-fragile absolute upper-bound assertion
on results.TimeToLastByte in the test, relying on the existing lower-bound and
TimeToFirstByte ordering assertions to validate the intended semantics. Keep the
ttlb count and remaining timing checks unchanged.
- Around line 107-115: Update pacedBody.Read to retain an offset into the
current chunk instead of advancing b.i after every read. Copy only the remaining
chunk bytes that fit in p, advance the offset by n, and move to the next chunk
only when the current chunk is fully consumed, preserving io.EOF behavior after
all chunks are exhausted.
In `@router/internal/traceclient/traceclient.go`:
- Around line 422-441: Update timedResponseBody.Read to detect wrapped EOF
errors with errors.Is(err, io.EOF) instead of direct equality, adding the errors
import while preserving the existing content-length completion logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e55b0cf3-7e0f-4f74-85cc-0feb77e69e0a
📒 Files selected for processing (16)
docs-website/router/configuration.mdxdocs-website/router/configuration/template-expressions.mdxdocs-website/router/metrics-and-monitoring.mdxdocs-website/router/metrics-and-monitoring/prometheus-metric-reference.mdxrouter-tests/observability/prometheus_test.gorouter-tests/observability/structured_logging_test.gorouter-tests/telemetry/connection_metrics_test.gorouter/internal/expr/expr.gorouter/internal/traceclient/traceclient.gorouter/internal/traceclient/traceclient_test.gorouter/pkg/config/config.schema.jsonrouter/pkg/metric/connection_measurements.gorouter/pkg/metric/connection_metric_store.gorouter/pkg/metric/noop_connection_metrics.gorouter/pkg/metric/oltp_connection_metric_store.gorouter/pkg/metric/prom_connection_metric_store.go
…-add-metric-request-timetolastbyte
| // This is always before the subgraph access log is written, because the | ||
| // engine fully reads and closes the body during the fetch load phase, which | ||
| // completes before the log is emitted. | ||
| if recorder != nil { |
There was a problem hiding this comment.
Can you inverse this if statement so it becomes an early return?
| } | ||
|
|
||
| func responseHasNoBody(req *http.Request, resp *http.Response) bool { | ||
| if req.Method == http.MethodHead || resp.Body == http.NoBody || resp.ContentLength == 0 { |
There was a problem hiding this comment.
Confused on why req is part of this function. Isn't it only about resp ? If its a response to an HTTP HEAD, shouldn't the body still be empty so you don't need to check the req method specifically?
| // lastByteMetricProvider is an optional extension implemented by the built-in | ||
| // OTLP and Prometheus providers. Keeping it separate avoids adding required | ||
| // methods to the exported ConnectionMetricProvider interface. | ||
| type lastByteMetricProvider interface { |
There was a problem hiding this comment.
Any particular reason you chose a new interface here? You could also move these methods to ConnectionMetricProvider and get rid of this new interface
| // LastByteMetricStore is an optional extension for first-to-last-byte transfer | ||
| // metrics. Implementations of ConnectionMetricStore that predate these metrics | ||
| // remain source-compatible. | ||
| type LastByteMetricStore interface { |
There was a problem hiding this comment.
Any particular reason you chose a new interface here? You could also move these methods to ConnectionMetricStore and get rid of this new interface
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.