metrics: per-session download goodput histogram#277
Merged
Conversation
Adds proxy.session.goodput, a Float64Histogram (By/s) recorded once at connection close = received bytes / connection seconds, for sessions that moved >= 1MB. Both TCP (conn.go) and UDP (packet_conn.go) accumulate per-connection received bytes and emit at Close; tagged with the receive direction plus the existing attribute set, so it's sliceable by track × cloud.region (resource attrs) × geo.country.iso_code (point attr). This is the measurement signal for the automatic bandit experimentation system (lantern-cloud): the evaluator compares a challenger track's median goodput against the incumbent's, per region×country, to decide big wins. Note: 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 >=1MB floor filters idle-dominated noise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new OpenTelemetry metric to capture per-session download throughput (“goodput”) as a histogram, emitted once on connection close, to support per-track/region/country comparisons in Lantern’s experimentation evaluation.
Changes:
- Introduces
proxy.session.goodput(Float64Histogram, unitBy/s) computed asreceived_bytes / connection_secondsfor sessions with ≥goodputMinBytes. - Tracks per-connection received bytes for both stream (
Conn) and packet (PacketConn) connections and records goodput onClose(). - Adds synctest-based unit coverage for above-threshold and below-threshold sessions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tracker/metrics/tracker.go | Adds goodputMinBytes and recordGoodput() emission logic. |
| tracker/metrics/metrics.go | Registers the new proxy.session.goodput histogram instrument. |
| tracker/metrics/conn.go | Accumulates per-connection received bytes and records goodput at close (TCP). |
| tracker/metrics/packet_conn.go | Accumulates per-connection received bytes and records goodput at close (UDP). |
| tracker/metrics/tracker_test.go | Adds tests verifying one-sample emission above threshold and none below. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- recordGoodput copies the attribute slice into a freshly-sized buffer before appending the direction tag, instead of appending onto AsSlice()'s result in place — removes any chance of sharing a backing array with a concurrent reporter (AsSlice reallocates today, but this is robust regardless). - The two goodput tests now defer server.Close() (matching the file's pattern) so an early failure can't leak the pipe end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Reword the goodput doc comments to "received bytes per second of connection lifetime" — the metric is bytes ÷ connection-lifetime seconds, and "per connection second" read ambiguously. - Change the proxy.session.goodput unit from "By/s" to "bytes/s" to match proxy.io's existing "bytes" spelling, keeping unit naming consistent within the metrics package (the metric has no consumers yet, so this is a free alignment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
|
@myleshorton should we do the same for http proxy? |
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 (lantern-cloud PR #2874). 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.What
proxy.session.goodput— aFloat64Histogram(unitBy/s) recorded once at connection close =received bytes / connection seconds, for sessions that moved ≥ 1 MB.conn.go) and UDP (packet_conn.go) now accumulate per-connection received bytes (atomic) and emit atClose(). UDP matters because hysteria2/tuic/wireguard are packet-based.network.io.direction=receiveplus the existing attribute set, so it's sliceable bytrack×cloud.region(resource attrs fromOTEL_RESOURCE_ATTRIBUTES) ×geo.country.iso_code(point attr) — 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.
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. "Active transfer time" is a possible future refinement.
Tests
TestSessionGoodput(≥1 MB session → one sample, value ≈ bytes/sec, receive tag) andTestSessionGoodputBelowThreshold(sub-threshold → no sample), mirroring the existing synctest harness.go build ./...clean;GOEXPERIMENT=synctest go test ./tracker/metrics/all green (run under go1.24.7 to match CI).🤖 Generated with Claude Code