Add Telemetry for call type, simultaneous tracks, and renegotiations

RESOLVED FIXED in Firefox 44

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
We'll count Renegotiations, call type (bitmask of Audio, Video and DataChannels), and max number of send and receive channels of each type (audio, video).
(Assignee)

Comment 1

3 years ago
Created attachment 8665155 [details] [diff] [review]
Add Telemetry for WebRTC call type, simultaneous tracks, and renegotiations
Attachment #8665155 - Flags: review?(docfaraday)

Comment 2

3 years ago
Comment on attachment 8665155 [details] [diff] [review]
Add Telemetry for WebRTC call type, simultaneous tracks, and renegotiations

Review of attachment 8665155 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +414,5 @@
>  #endif
> +  for (size_t i = 0; i < SdpMediaSection::kMediaTypes; i++) {
> +    mMaxReceiving[i] = 0;
> +    mMaxSending[i] = 0;
> +  }

memset?

@@ +2591,5 @@
>          fireNegotiationNeeded = true;
>        }
>      }
> +
> +    // Telemetry: record info on the current state of streams/renegotiations/etc

This code will run when rollback happens (which will cause a transition to stable). This is probably not what we want.

@@ +2592,5 @@
>        }
>      }
> +
> +    // Telemetry: record info on the current state of streams/renegotiations/etc
> +    if (mSignalingHadBeenStable) {

Seems like this could be simpler if we just counted the number of negotiations, and then subtracted one when recording the value in telemetry.

@@ +2617,5 @@
>    }
>  
>    if (mSignalingState == PCImplSignalingState::SignalingClosed) {
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +    // Report end-of-call Telemetry

Why not put this in CloseInt (probably broken out into a function like RecordEndOfCallTelemetry())?

@@ +2637,5 @@
> +    // DataChannels appear in both Sending and Receiving
> +    Telemetry::Accumulate(mIsLoop ? Telemetry::LOOP_DATACHANNEL_NEGOTIATED :
> +                                    Telemetry::WEBRTC_DATACHANNEL_NEGOTIATED,
> +                          mMaxSending[SdpMediaSection::MediaType::kApplication]);
> +    // Enumerated/bitmask: 0 = Audio, 1 = Video, 4 = DataChannel

Let's declare the flags in code instead of in a comment. Also, we want 1, 2, 4 here not 0, 1, 4.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +549,5 @@
>  
>    return NS_OK;
>  }
>  
> +void PeerConnectionMedia::CountMediaPipelines(

I don't understand why this lives in PCM; none of PCM's state is actually used here. PCI is the primary user of the JsepSession, as well as the owner of the counter arrays. If we want this kind of helper function, I would expect it to be a non-virtual function in JsepSession.

@@ +552,5 @@
>  
> +void PeerConnectionMedia::CountMediaPipelines(
> +  const JsepSession& session,
> +  uint16_t (&receiving)[SdpMediaSection::kMediaTypes],
> +  uint16_t (&sending)[SdpMediaSection::kMediaTypes]) {

I'm not wild about the use of arrays as function params, but maybe the syntax you've used makes sizeof(receiving) result in the right value? I could see someone trying to replace your loop below with memset, which could trip over this gotcha.

::: toolkit/components/telemetry/Histograms.json
@@ +6570,5 @@
> +  },
> +  "WEBRTC_MAX_AUDIO_SEND_TRACK": {
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": "20",

I'm not sure I understand why these limits are higher for audio than they are for video. Maybe because large video conferences tend to have some percentage of attendees that send audio only?
Attachment #8665155 - Flags: review?(docfaraday)
(Assignee)

Comment 3

3 years ago
Created attachment 8665987 [details] [diff] [review]
Add Telemetry for WebRTC call type, simultaneous tracks, and renegotiations

Yes, there's a difference in the realistic number of 'send' tracks for video and audio, and even these limits in the telemetry are likely massive overkill for outgoing video.  And we can always change this easily.  In general, for non-mesh you'll send one, or at most a handful of tracks of audio or video.  For mesh, you'll send 1 per person in the mesh, but that has a realistic limit for video around maybe 6 or perhaps 8.  For pure audio mesh, limits are higher, but more than 12 or ***maybe*** 15 or 20 are very unlikely
Attachment #8665987 - Flags: review?(docfaraday)
(Assignee)

Updated

3 years ago
Attachment #8665155 - Attachment is obsolete: true

Comment 4

3 years ago
Comment on attachment 8665987 [details] [diff] [review]
Add Telemetry for WebRTC call type, simultaneous tracks, and renegotiations

Review of attachment 8665987 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a few small modifications.

::: media/webrtc/signaling/src/jsep/JsepSession.h
@@ +172,5 @@
>  
>    virtual bool AllLocalTracksAreAssigned() const = 0;
>  
> +  void
> +  CountMediaPipelines(uint16_t (&receiving)[SdpMediaSection::kMediaTypes],

Let's call this CountTracks.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2634,5 @@
>          fireNegotiationNeeded = true;
>        }
>      }
> +
> +    // Telemetry: record info on the current state of streams/renegotiations/etc

Running this code on rollback isn't what we want, although I doubt it will break anything right now. It might encourage more code to land down here though.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +270,5 @@
>  
> +// Bitmask used for WEBRTC_CALL_TYPE telemetry reporting
> +#define kAudioTypeMask 1
> +#define kVideoTypeMask 2
> +#define kDataChannelTypeMask 4

If you made these static const unsigned inside RecordEndOfCallTelemetry, it would explain what they're for just as well as the comment, and would have this stuff all in one place.
Attachment #8665987 - Flags: review?(docfaraday) → review+
https://hg.mozilla.org/mozilla-central/rev/0d841bed4209
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.