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.
3 participants
No description provided.