Closed Bug 1207824 Opened 9 years ago Closed 9 years ago

Add Telemetry for call type, simultaneous tracks, and renegotiations

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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).
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)
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)
Attachment #8665155 - Attachment is obsolete: true
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: