metrics: emit session goodput for all sessions, not just >=1MB (fix false experiment starvation) - #678
Conversation
proxy.session.goodput was recorded once per session at close, but only for sessions that moved >= 1MB in the receive direction. The bandit experiment evaluator counts goodput samples per (track, country) as a starvation signal and retires challengers with < 10 samples in 48h. On prod ~79% of challengers were being false-retired as "starved" while serving large real traffic. The 1MB floor is applied to the receive (client->proxy / upload) side, which is the small direction of a session. Prod (48h) averages ~22 KB received and ~275 KB sent per session across 146M sessions -- both far below 1MB -- so small-but-real sessions (probes, connectivity checks, blocked-then-retry, small pages, common in censored markets) recorded a bandit.callback but no goodput sample, and the evaluator counted ~0. Emit goodput for any session with recvBytes > 0 and duration > 0 so the sample count tracks real traffic. Direction, the (track, country) point attributes (#675), and the histogram definition are unchanged. The per-second rate is noisier for tiny sessions, but the evaluator compares per-stratum p50 medians which are robust to that tail. Also add otelinstrument.ResetForTest to fix a latent order-dependent leak: Initialize()'s sync.Once bound the goodput histogram to the first test's meter provider, so only the first positive-emission test in the process observed samples. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSession goodput no longer requires a 1 MB minimum and now records positive-byte sessions. Tests rebind OTEL instrumentation between manual readers and cover small-session and zero-byte behavior. ChangesSession goodput instrumentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
instrument/otelinstrument/otelinstrument.go (1)
50-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument concurrency hazard in test reset.
ResetForTestmutates global variables (initOnce,meter, and instruments) without synchronization. While safe for sequential tests, this will cause data races ift.Parallel()is ever used in tests that rely on this package. Consider noting this limitation in the docstring to prevent future test flakiness.🤖 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/otelinstrument/otelinstrument.go` around lines 50 - 60, Update the ResetForTest docstring to explicitly state that it mutates shared global initialization state and instruments without synchronization, so it is only safe for sequential tests and must not be used with t.Parallel().
🤖 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 `@instrument/otelinstrument/otelinstrument.go`:
- Around line 50-60: Update the ResetForTest docstring to explicitly state that
it mutates shared global initialization state and instruments without
synchronization, so it is only safe for sequential tests and must not be used
with t.Parallel().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3aab0fc-8c3e-42ef-bbdc-1cc0dcb4d28d
📒 Files selected for processing (3)
instrument/goodput_test.goinstrument/instrument.goinstrument/otelinstrument/otelinstrument.go
Address CodeRabbit review: ResetForTest mutates package-global init state without synchronization, so document that it is sequential-tests only and must not be used with t.Parallel(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai addressed the |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Pull request overview
This PR adjusts proxy.session.goodput emission so it’s recorded for all real sessions (any recvBytes > 0 and duration > 0) rather than only sessions exceeding a 1MB receive threshold, preventing the experiment evaluator from falsely treating healthy challengers as “starved” due to missing goodput samples.
Changes:
- Remove the per-session 1MB goodput emission floor and emit goodput for any session with positive received bytes and duration.
- Add/adjust tests to cover small sessions, zero bytes, and zero duration.
- Add
otelinstrument.ResetForTest()to rebind OTEL instruments to the active test meter provider, avoiding order-dependent test failures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| instrument/instrument.go | Removes the byte floor and updates SessionGoodput behavior and rationale documentation. |
| instrument/goodput_test.go | Adds coverage for small-session emission and ensures tests can reliably observe OTEL metrics. |
| instrument/otelinstrument/otelinstrument.go | Adds a reset hook to re-initialize OTEL instruments against the current global meter provider for tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t doc Address Copilot review: - ResetForTest now refuses to run outside a `go test` binary (flag.Lookup "test.v"), so a stray production call can't re-run initialization and race live metric use. - Update the proxy.session.goodput histogram comment/description that still referenced the removed goodputMinBytes floor and mislabeled the metric as "download"; it now describes the receive (client->proxy) direction with no byte floor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What & why
proxy.session.goodputis recorded once per session at connection close, but only for sessions that moved ≥ 1 MB in thereceivedirection:The bandit experiment evaluator (lantern-cloud) reads the count of goodput samples per
(track, country)as a starvation signal: a challenger with< 10samples in 48h is retired ("starved / infra-broken", 0 samples → aborted). On prod ~79% of challengers were being false-retired this way while serving large real traffic.Root cause (confirmed): the 1 MB per-session floor is applied to the
receivedirection, which is the small (client→proxy / upload) side of a session, and the floor is ~1–2 orders of magnitude larger than a typical session moves. Small-but-real sessions — probes, connectivity checks, blocked-then-retry, small pages, which dominate censored markets — record abandit.callbackbut no goodput sample, so the evaluator counts ≈0 goodput and kills healthy challengers.This PR removes the byte floor: goodput is now emitted for any session with
recvBytes > 0andduration > 0, so the sample count tracks real traffic. Direction,(track, country)point-attribute slicing (#675), and the histogram definition are unchanged.Prod evidence (SigNoz, 48h)
Two proxy services emit goodput:
http-proxy-lantern(the cloud http-proxy fleet, nodeployment.environmenttag) andvps-proxy(env=prod; runs both http-proxy and the sing-box/lantern-box binary).bandit.callbacksis emitted by theapiservice. Figures below are across both proxy services (no env filter) — adeployment.environment='prod'filter silently drops the largerhttp-proxy-lanternfleet.proxy.iobytes by direction (both services combined):transmit(proxy→client, download) = 51.9 TBreceive(client→proxy, upload) = 4.81 TB → transmit:receive ≈ 10.8 : 1 (http-proxy-lanternalone 12.4:1)Average bytes per connection (
http-proxy-lantern, the only service withproxy.connections; 146.5 M sessions/48h):Both averages are far below the 1 MB floor; the floored direction (upload) is ~45× under it, and even download is ~3.6× under. Only 0.042% of connections (61,903 / 146.5 M) ever cleared the floor. Overall goodput samples (166,208) are 0.43% of
bandit.callbacks(38.6 M).Callbacks vs goodput samples, the false-starved challengers (all on
service.name=http-proxy-lantern, i.e. the http-proxy binary this PR fixes):bandit.callbacksproxy.session.goodput.count(Matches the original prod diagnosis table.)
Audit answers
Q1 — Is the 1 MB floor the dominant cause? Every close path checked.
reporting.goproxiedBytesReporter, onfinal=true(connection close), before the zero-delta early return, with cumulativestats.RecvTotal/stats.Duration. It fires once for every closed measured connection.bwReporting.wrapper) is added viasrv.AddListenerWrappersand applied byserver.serve()to every accepted connection, for every protocol listener ingetProtoListenersArgs(https/tls, https_multiplex, tlsmasq, starbridge, broflake, algeneva, kcp, quic_ietf, shadowsocks, shadowsocks_multiplex, water, vmess) and in both the multipath and non-multipath serve paths. For multiplexed transports the measured wrapper is outermost and wraps the per-session logical stream, soDurationis per-session andfinalfires per session.finalfires.ListenAndServeENHTTP(encapsulated-HTTP mode) is a mutually-exclusive alternate serve path that uses a plainhttp.Serverand does not apply the measured wrapper, so it emits neitherproxy.ionor goodput. Niche deployment; flagged, not addressed here.Q2 — Image / protocol coverage (which binaries emit goodput):
Serving binary is decided by a track's
docker_image_ref, not the wire protocol name (shadowsocks/vmess/algeneva/water have listeners on both binaries). In lantern-cloud (origin/main):cmd/api/pcfg/pcfg.gogenerateLaunchConfig(pcfg.go:136) builds a typed launch config;cmd/api/proxyini/proxyini.goBuild(proxyini.go:73) renders it as an http-proxy INI, or rejects a sing-box config (proxyini.go:208-212) → runs on lantern-box. Sing-box membership:IsSingboxProtocol(pcfg/singbox.go:28-37). Per-track image classification:track.go:180-201; SQL split ruleproxy_infrastructure.sql:1692-1712.service.namehttp-proxy-lantern+ part ofvps-proxy): tls, tlsmasq, shadowsocks (protocol 1), vmess, starbridge, algeneva, water, broflake. Emits goodput,trackvisible as a point attr (metrics: emit goodputtrackas a point attribute so the bandit evaluator can slice it #675), 1 MB floor. Fixed by this PR.service.name=vps-proxy): reflex, samizdat, meek, unbounded, wireguard, hysteria2, vless, trojan, amnezia, sing-box-native ss/vmess. Also emitsproxy.session.goodput(tracker/metrics/metrics.go:66-71; recorded attracker/metrics/tracker.go:142-155) with the identicalgoodputMinBytes = 1_000_000(tracker.go:28,143). In prod its tracks under-emit the same way (e.g. samizdat-pro-alicloud, hysteria2-*, vless-*). Needs the same floor fix — separate PR in lantern-box.git grep radianceover lantern-cloudcmd/api,cmd/phost,tf/= 0 hits) and emits zeroproxy.session.goodput— its only "goodput" is a local KB/s CLI probe incmd/residential-urltest(main.go:237,262); serving telemetry recordsconnectionDurationonly. Not part of the experiment fleet.getlantern/http-proxy-lanternrepo: not deployed — absent from the CI/deploy allowlist[automation, flashlight, http-proxy, lantern-cloud, lantern-box](tf/_modules/foundation/lanternet/cicd.tf:35-41); the deployed image ishttp-proxy:latest(cmd/phost/main.go:50). It is a byte-identical mirror of this repo (same HEAD SHA), so no divergent goodput path.trackresource-only / reader-invisible; prod refutes this —trackresolves as a queryable label onvps-proxygoodput, andGoodputByStratumhas no service filter, so it matches both. The floor is the sole active cause.)The 5 false-starved tracks are all http-proxy (Class A) — proven empirically: three show non-zero goodput (375 / 4,633 / 2), which a resource-only-track binary could never do (the reader's point-attr
trackfilter would read exactly 0 in every stratum). The two 0-rows are the floor on low-volume http-proxy tracks, same mechanism as the tlsmasq row that yielded 2 from 6,636 callbacks.Q3 — Are
recvBytes/durationpopulated for all protocols?measured.Connwrapping the client socket), so there's no per-protocol silent-zero within http-proxy.recvBytes = stats.RecvTotalis the client→proxy (upload) direction (taggedreceive), the smaller side — even though the code/histogram describe it as "download goodput". This is internally consistent across both emitters and the reader (all pinned toreceive), so it's a fair relative signal and I preserved it. But it does mean the metric measures upload throughput, not download; if the team wants download as the quality signal that's a coordinated emitter+reader change (see follow-ups).Changes
instrument/instrument.go: removegoodputMinBytes; emit goodput for any session withrecvBytes > 0 && duration > 0; rewrite the doc comment with the rationale + prod numbers.instrument/goodput_test.go: addTestSessionGoodputSmallSession(20 KB session now records — the core fix) andTestSessionGoodputZeroBytes; keep the ≥1 MB and zero-duration cases.instrument/otelinstrument/otelinstrument.go: addResetForTest()— fixes a latent, order-dependent test-harness leak (thesync.OnceinInitialize()bound the goodput histogram to the first test's meter provider, so only the first positive-emission test in the process ever observed samples).Test evidence
All four pass in any order (verified standalone).
go vet ./instrument/... ./instrument/otelinstrument/...clean.go build ./...fails only inside the unrelated CGO depgithub.com/anacrolix/go-libutp(typedef uint8 bool, a C-toolchain issue) on both this branch and origin/main; the changed packages build clean.Volume / cardinality
Removing the floor raises the number of histogram records (~846/s fleet-wide, same as
proxy.connections) but not series cardinality — the label set(track, geo.country.iso_code, network.io.direction)is unchanged and low-cardinality. Distinct from the measurement-table volume concern in getlantern/engineering#3691.Cross-repo follow-ups (NOT in this PR — coordinate)
tracker/metrics/tracker.go:28,143): apply the identical floor removal — samegoodputMinBytes = 1_000_000, and it serves the sing-box experiment protocols (reflex, samizdat, hysteria2, vless, trojan, amnezia, wireguard, meek, sing-box-native ss/vmess), all under-emitting. (Also worth confirming lantern-box carries atrackpoint attribute — its code setstrackon the OTEL resource, though in prod SigNoz exposes it as a queryable label today.)cmd/api/jobs/experiment_evaluator_worker.go,settingExperimentStarvationMinSamples): gate starvation onbandit.callbacks/attempts rather than goodput sample count — a challenger with thousands of callbacks is not starved regardless of goodput emission. (A worktree for this already exists.)network.io.direction='receive'filter.