Experimental WARP - #4649
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| func IncrementParticipantRtcConnected(connected uint32) { | ||
| if connected > 0 { | ||
| participantRTCConnected.Add(uint64(connected)) | ||
| promParticipantJoin.WithLabelValues("rtc_connected").Add(float64(connected)) | ||
| promParticipantJoin.WithLabelValues("rtc_connected", "").Add(float64(connected)) | ||
| } | ||
| } | ||
|
|
||
| func IncrementParticipantRtcActive(active uint32) { | ||
| func IncrementParticipantRtcActive(active uint32, warp bool) { | ||
| if active > 0 { | ||
| participantRTCActive.Add(uint64(active)) | ||
| promParticipantJoin.WithLabelValues("rtc_active").Add(float64(active)) | ||
| promParticipantJoin.WithLabelValues("rtc_active", strconv.FormatBool(warp)).Add(float64(active)) | ||
| } | ||
| } | ||
|
|
||
| func IncrementParticipantRtcCanceled(canceled uint64) { | ||
| func IncrementParticipantRtcCanceled(canceled uint64, warp bool) { | ||
| if canceled > 0 { | ||
| participantRTCCanceled.Add(canceled) | ||
| promParticipantJoin.WithLabelValues("rtc_canceled").Add(float64(canceled)) | ||
| promParticipantJoin.WithLabelValues("rtc_canceled", strconv.FormatBool(warp)).Add(float64(canceled)) | ||
| } |
There was a problem hiding this comment.
🔍 Inconsistent warp label values across prometheus metric states
The promParticipantJoin counter now has a warp label, but its values are inconsistent across states: signal_connected, signal_failed, rtc_connected, etc. all use "" (empty string), while rtc_active and rtc_canceled use strconv.FormatBool(warp) producing "true" or "false". This means the warp label has three distinct values ("", "true", "false"), which could make Prometheus queries and dashboards harder to write correctly (e.g., sum by (state) would need to account for all three). Consider whether using "false" instead of "" for non-warp-aware states would simplify querying.
(Refers to lines 299-355)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
We only decide the warp state when trying to establish the peer connection. The empty value of warp represents an unknown state in those functions, as we don't know the warp state there.
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
lgtm!
How different are the pion forks? Would it be hard to keep syncing?
Also, the Devin suggestion looks good to apply.
The pion/webrtc,ice's changes are clean and not hard to apply when syncing. Pion/dtls has more lines, but the dtls1.2 is stable and the development is focus on dtls1.3 now, so we don't need to sync it frequently. |
| google.golang.org/grpc v1.81.1 // indirect | ||
| ) | ||
|
|
||
| replace github.com/pion/webrtc/v4 => github.com/livekit/webrtc-pion/v4 v4.2.16-warp.2 |
There was a problem hiding this comment.
Could you make sure these forks have proper ownership (cs-devs or devs)?
| ConstLabels: prometheus.Labels{"node_id": nodeID, "node_type": nodeType.String()}, | ||
| Buckets: prometheus.ExponentialBucketsRange(100, 10000, 15), | ||
| }, []string{"protocol_version"}) | ||
| }, []string{"protocol_version", "warp"}) |
There was a problem hiding this comment.
🔍 Breaking change to promSessionStartTime histogram labels
Adding the "warp" label to promSessionStartTime at pkg/telemetry/prometheus/rooms.go:116 changes the metric's label set. Existing Prometheus queries, alerts, and Grafana dashboards that reference livekit_session_start_time_ms without the warp label will need to be updated. The old time series (without the warp label) and new time series (with it) will coexist during a rolling upgrade, which could cause double-counting in aggregation queries until the old series expire. This is a deployment consideration rather than a code bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }, | ||
| }) | ||
| prometheus.IncrementParticipantRtcCanceled(1) | ||
| prometheus.IncrementParticipantRtcCanceled(1, false) |
There was a problem hiding this comment.
🟡 Aborted connection attempts are always counted as non-accelerated, skewing the new experiment metric
Two failure paths label aborted connection attempts as not using the new acceleration mode (IncrementParticipantRtcCanceled(1, false) at pkg/service/roommanager.go:309 and pkg/service/roommanager.go:422) even when the server has it turned on, so the failure counts for the accelerated group are undercounted.
Impact: The dashboards comparing accelerated vs. normal connections show fewer aborted attempts for the accelerated group than actually occurred, making the experiment look better than it is.
How the label is chosen in each cancel path
All other cancel/failure sites use the actual per-session value: enableWarp (computed at pkg/service/roommanager.go:487) or participant.IsWarpEnabled() (pkg/service/roommanager.go:362, pkg/service/roommanager.go:386). The two sites flagged here hardcode false:
- line 309: room creation failed, no participant exists yet — the session's warp state is still knowable from
r.config.RTC.EnableWarp(combined withuseOneShotSignallingMode). - line 422: reconnect requested but participant missing from the room — same.
Since promParticipantJoin is now keyed by {state, warp} (pkg/telemetry/prometheus/packets.go:152), these increments land in the warp="false" series regardless of the deployment configuration.
Prompt for agents
In pkg/service/roommanager.go StartSession, the two prometheus.IncrementParticipantRtcCanceled calls at lines 309 and 422 pass a hardcoded false for the new warp label, while every other call site passes the session's actual value (enableWarp computed at line 487 or participant.IsWarpEnabled()). These two paths (room creation failure, and reconnect with participant missing from room) still know the effective warp setting from !useOneShotSignallingMode && r.config.RTC.EnableWarp. Consider hoisting that computation above the early-return paths and using it for both calls so warp-labelled cancel counters are accurate.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
It is canceled by state_mismatch, so set warp:false here doesn't matter.
3 participants
No description provided.