1017291 - We should record rejected remote candidates in telemetry

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

In some of my testing, I've found that apprtc has a tendency to reorder the remote SDP and the remote trickle candidates, so that sometimes trickle candidates arrive first. In this case, we drop the candidates on the floor, which can cause ICE to fail. It would be nice to know how often remote trickle candidates are dropped.

Comment 1

[Byron Campen [:bwc]]

Attachment: Keep track of the number of errors in AddIceCandidate before ICE completes, and record this number in telemetry in the success and failure cases separately.

Initial cut.

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8430425 [details] [diff] [review]
Keep track of the number of errors in AddIceCandidate before ICE completes, and record this number in telemetry in the success and failure cases separately.

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

Note that I've deliberately only taken into account the number of errors before ICE completes; I don't want subsequent errors to add noise to the statistics.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 8430425 [details] [diff] [review]
Keep track of the number of errors in AddIceCandidate before ICE completes, and record this number in telemetry in the success and failure cases separately.

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

Clearing the r? bit until I understand why this is 9. I'll r+ if there is a good answer.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +723,5 @@
>    bool mHaveDataStream;
>  
>    // Holder for error messages from parsing SDP
>    std::vector<std::string> mSDPParseErrorMessages;
> +  unsigned int mAddCandidateErrorCount;

Suggest using a concrete type like uint32_t

::: toolkit/components/telemetry/Histograms.json
@@ +5272,5 @@
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": "10",
> +    "n_buckets": "9",
> +    "description": "The number of times AddIceCandidate failed on a given PeerConnection, given that ICE succeeded."

I'm not an expert here, but why is this stopping at 9? Isn't it possible to have a lot more candidates?

Comment 4

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #3)
> Comment on attachment 8430425 [details] [diff] [review]
> Keep track of the number of errors in AddIceCandidate before ICE completes,
> and record this number in telemetry in the success and failure cases
> separately.
> 
> Review of attachment 8430425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing the r? bit until I understand why this is 9. I'll r+ if there is a
> good answer.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +723,5 @@
> >    bool mHaveDataStream;
> >  
> >    // Holder for error messages from parsing SDP
> >    std::vector<std::string> mSDPParseErrorMessages;
> > +  unsigned int mAddCandidateErrorCount;
> 
> Suggest using a concrete type like uint32_t
> 

   If our coding convention for string stuff didn't force the use of format strings (if we ever want to printf this we have to cast or use an ugly macro), or if I actually cared about the particulars of type width here, I would agree.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +5272,5 @@
> > +    "expires_in_version": "never",
> > +    "kind": "linear",
> > +    "high": "10",
> > +    "n_buckets": "9",
> > +    "description": "The number of times AddIceCandidate failed on a given PeerConnection, given that ICE succeeded."
> 
> I'm not an expert here, but why is this stopping at 9? Isn't it possible to
> have a lot more candidates?

This limit was mostly arbitrary, based on what kind of granularity I felt was actionable (what do we do differently if samples >9 fall into one bucket, or spread across more buckets?). But, buckets are basically free, and might make the histogram easier to look at, so I'm happy to add more. I just don't want to end up with a histogram with razor-thin bars because some weirdo managed to pump 2000 garbage candidates at us. I think any more than 30 starts to make the histogram hard to read, does that sound sane as a limit?

Comment 5

[Byron Campen [:bwc]]

Attachment: Keep track of the number of errors in AddIceCandidate before ICE completes, and record this number in telemetry in the success and failure cases separately.

Now with more buckets

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 8430857 [details] [diff] [review]
Keep track of the number of errors in AddIceCandidate before ICE completes, and record this number in telemetry in the success and failure cases separately.

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

lgtm

Comment 7

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=ca0b9f1e76ea

Comment 8

[Byron Campen [:bwc]]

Try looks good enough for me.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcbe7b1dec2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fcbe7b1dec2