metrics: per-session download goodput histogram#674
Merged
Conversation
Adds proxy.session.goodput, a Float64Histogram (unit bytes/s) recorded once at connection close = received bytes / connection seconds, for sessions that moved >= 1 MB. This is the measurement signal for Lantern's automatic bandit experimentation system: to decide whether a challenger track actually delivers a big win, the evaluator needs a real per-session throughput signal it can compare between challenger and incumbent, per region x country. proxy.io (a byte counter) can't give a rate/distribution; this adds one. Tagged with network.io.direction=receive plus geo.country.iso_code (point attr), so it's sliceable by track x dc/region (resource attrs from buildResource) x country -- exactly the strata the evaluator compares. No device_id tag (cardinality). Unlike lantern-box#277, which wraps Conn/PacketConn to accumulate rx bytes and emit at Close(), http-proxy-lantern already measures per-connection bytes and duration via the measured listener: the reporting callback receives cumulative stats and a final=true flag at close. So goodput is recorded there, before the zero-delta early return so a session idle during its last reporting interval is still counted. Caveat (documented in code): duration is connection open time (includes idle), so goodput is a floor on true transfer speed. Both experiment arms are measured identically, so it's a fair relative signal, and the >=1 MB floor filters idle-dominated noise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds session goodput instrumentation by registering a new ChangesSession Goodput Instrumentation
Sequence Diagram(s)sequenceDiagram
participant proxiedBytesReporter
participant defaultInstrumentSessionGoodput as defaultInstrument.SessionGoodput
participant sessionGoodputHistogram as otelinstrument.SessionGoodput
proxiedBytesReporter->>defaultInstrumentSessionGoodput: SessionGoodput(ctx, recvBytes, duration, clientIP)
defaultInstrumentSessionGoodput->>sessionGoodputHistogram: record goodput with attrs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
instrument/goodput_test.go (1)
49-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the
geo.country.iso_codeattribute to fully cover the metric contract.The positive-path test currently validates only
network.io.direction; adding a country-attribute assertion would better lock the intended emitted shape.🤖 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 `@instrument/goodput_test.go` around lines 49 - 51, The positive-path check in the goodput test only verifies network.io.direction, so the emitted metric shape is not fully covered. Update the test that uses extractHistogramAttrs and assert the geo.country.iso_code attribute as well, alongside the existing proxy.session.goodput and network.io.direction checks, so the contract is validated in the same test.
🤖 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.
Inline comments:
In `@instrument/goodput_test.go`:
- Around line 20-29: newGoodputInstrument mutates the global OTEL meter provider
without restoring it, which can leak the manual-reader provider into later
tests. In newGoodputInstrument, save the current provider from
sdkotel.GetMeterProvider before calling sdkotel.SetMeterProvider, then use
t.Cleanup to restore that original provider after the test finishes. Keep the
change localized to newGoodputInstrument and the MeterProvider setup around
sdkmetric.NewManualReader/NewMeterProvider.
---
Nitpick comments:
In `@instrument/goodput_test.go`:
- Around line 49-51: The positive-path check in the goodput test only verifies
network.io.direction, so the emitted metric shape is not fully covered. Update
the test that uses extractHistogramAttrs and assert the geo.country.iso_code
attribute as well, alongside the existing proxy.session.goodput and
network.io.direction checks, so the contract is validated in the same test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cee7f2ce-83d2-4324-b3ab-2e668290e5ec
📒 Files selected for processing (4)
instrument/goodput_test.goinstrument/instrument.goinstrument/otelinstrument/otelinstrument.goreporting.go
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a per-session download “goodput” telemetry signal (proxy.session.goodput) so the bandit evaluator can compare per-track throughput distributions (e.g., p50) across region/country strata using a single sample emitted at connection close.
Changes:
- Emit a
Float64Histogramsample at connection close withrecv_bytes / connection_open_secondsfor sessions with ≥1,000,000 received bytes. - Wire the emission into the measured listener reporting callback (so final/idle-last-interval sessions are still counted).
- Add unit tests covering thresholding and zero-duration guard.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| reporting.go | Records session goodput at connection close from cumulative measured stats. |
| instrument/otelinstrument/otelinstrument.go | Defines the proxy.session.goodput Float64Histogram with unit bytes/s. |
| instrument/instrument.go | Adds SessionGoodput() to the instrumentation interface and implements recording with geo + direction attrs. |
| instrument/goodput_test.go | Adds tests for goodput emission, threshold behavior, and zero-duration guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- reporting.go: skip client IP parsing on idle, non-final reporting intervals (return on zero-delta before parsing), and use a safe type assertion for ctx[common.ClientIP]. Avoids the per-interval overhead regression introduced by recording goodput at close. - goodput_test.go: restore the global meter provider via t.Cleanup so the manual-reader provider doesn't leak into other tests. - goodput_test.go: also assert the geo.country.iso_code point attribute is present, fully covering the emitted metric contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Why
The measurement signal for Lantern's automatic bandit experimentation system. To decide whether a challenger track (a new protocol/datacenter combination) actually delivers a big win, the evaluator needs a real per-session throughput signal it can compare between the challenger track and the incumbent, per region×country.
proxy.io(a byte counter) can't give a rate/distribution; this adds one.Port of getlantern/lantern-box#277 to http-proxy-lantern.
What
proxy.session.goodput— aFloat64Histogram(unitbytes/s) recorded once at connection close =received bytes / connection seconds, for sessions that moved ≥ 1 MB.network.io.direction=receiveplusgeo.country.iso_code(point attr), so it's sliceable bytrack×dc/region (resource attrs frombuildResource()) × country — exactly the strata the experiment evaluator compares.device_idtag (cardinality).Since the challenger and control are separate tracks, the arm is the track — no per-connection experiment tagging needed; the evaluator queries this histogram's p50 by track name per stratum.
How it differs from lantern-box#277
lantern-box wraps
Conn/PacketConnto accumulate per-connection rx bytes (atomic) and emits atClose(). http-proxy-lantern already measures per-connection bytes and duration via themeasuredlistener: the reporting callback (reporting.go) receives cumulativestats(withRecvTotalandDuration) and afinal=trueflag at close. So goodput is recorded there — before the zero-delta early return, so a session that was idle during its last reporting interval is still counted.Caveat (documented in code)
Duration is connection open time (includes idle), so goodput is a floor on true transfer speed. Both experiment arms are measured identically, so it's a fair relative signal, and the ≥1 MB floor filters idle-dominated noise.
Tests
TestSessionGoodput(≥1 MB session → one sample, value ≈ bytes/sec, receive tag),TestSessionGoodputBelowThreshold(sub-threshold → no sample), andTestSessionGoodputZeroDuration(divide-by-zero guard).go test ./instrument/green.🤖 Generated with Claude Code
Summary by CodeRabbit