874175 - Add Telemetry probe for ICE completion success rate

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Timothy B. Terriberry (:derf)]

As a first stab at adding telemtry, we should add a probe to count the number of successfully established ICE connections vs. the number attempted.

Comment 1

[Ben Brittain (:bbrittain)]

Attachment: Adds telemetry for ice success rates

Comment 2

[Timothy B. Terriberry (:derf)]

(In reply to Ben Brittain (:bbrittain) from comment #1)
> Created attachment 752398 [details] [diff] [review]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e8f8db977c16

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 752398 [details] [diff] [review]
Adds telemetry for ice success rates

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

r-, but it should be easy to fix this up.

::: dom/media/PeerConnection.js
@@ +934,5 @@
>      // we go ahead and trigger a call of onicecandidate here.
>      // This is to provide some level of compatibility with
>      // scripts that expect this behavior (which is how Chrome
>      // signals that no further trickle candidates will be sent).
> +    // TODO: This needs to be removed when Bug 842459 lands. 

Bug 842549.

@@ +935,5 @@
>      // This is to provide some level of compatibility with
>      // scripts that expect this behavior (which is how Chrome
>      // signals that no further trickle candidates will be sent).
> +    // TODO: This needs to be removed when Bug 842459 lands. 
> +    // TODO: Fix that bug number!

That probably shouldn't be in this patch, though, as it's unrelated to this change. Feel free to upload a separate patch to bug 856425, which added this comment, to fix this.

@@ +998,5 @@
>          this.callCB(this._ongatheringchange, "gathering");
>          break;
>        case Ci.IPeerConnection.kIceConnected:
>          // ICE gathering complete.
> +        histogram.add(0);

IF WEBRTC_ICE_SUCCESS_RATE has kind "boolean", I think the value you add should be true/false.

In which case completion should probably be true, and failure false.

::: toolkit/components/telemetry/Histograms.json
@@ +3313,5 @@
>      "description": "The time (in milliseconds) that it took a 'detach' request to go round trip."
> +  },
> +  "WEBRTC_ICE_SUCCESS_RATE":{
> +    "kind": "boolean",
> +    "description": "Number of sent ICE connections vs sucessful ICE connections"

What you're actually measuring is successful ICE connections vs. failed ICE connections.

Also, fix the spelling of "successful" and punctuate properly (periods after "vs" and at the end of the phrase).

Comment 4

[Ben Brittain (:bbrittain)]

Attachment: Adds telemetry for ice success rates; patch2

Comment 5

[Ben Brittain (:bbrittain)]

contrary to the name of the boolean telemetry type, it takes 0/1 not false/true

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 752398 [details] [diff] [review]
Adds telemetry for ice success rates

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

I have nothing to add to Timothy's review, other than to note that histogram.add() for a boolean histogram will work just fine with a numeric or boolean arg.  A boolean arg is generally clearer and IMHO better in this case.

Comment 7

[Timothy B. Terriberry (:derf)]

Comment on attachment 752818 [details] [diff] [review]
Adds telemetry for ice success rates; patch2

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

r=me with the one extraneous change removed. I'll leave it up to you whether you want to use 0/1 or true/false, since froydnj says they both work.

Don't forget to add a "r=derf" to the end of the checkin message before asking for checkin.

::: dom/media/PeerConnection.js
@@ +934,5 @@
>      // we go ahead and trigger a call of onicecandidate here.
>      // This is to provide some level of compatibility with
>      // scripts that expect this behavior (which is how Chrome
>      // signals that no further trickle candidates will be sent).
> +    // TODO: This needs to be removed when Bug 842459 lands. 

Please don't add trailing whitespace here.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Ben Brittain (:bbrittain) from comment #5)
> contrary to the name of the boolean telemetry type, it takes 0/1 not
> false/true

Where do you see this?  I just tested in-browser to confirm and the .add method does accept true/false in addition to 0/1.

Comment 9

[Ben Brittain (:bbrittain)]

Attachment: Adds telemetry for ice success rates; r=derf

I had originally tried true/false and not had it work. It must have been an unrelated problem. sorry about the confusion.

Comment 10

[Timothy B. Terriberry (:derf)]

Comment on attachment 752854 [details] [diff] [review]
Adds telemetry for ice success rates; r=derf

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

r=me again, but don't forget to add a commit message with the bug number and reviewer.
See <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>.

::: toolkit/components/telemetry/Histograms.json
@@ +3310,5 @@
>      "kind": "exponential",
>      "high": "10000",
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'detach' request to go round trip."
> +  },

Note that this will require rebasing.

Comment 11

[Maire Reavy [:mreavy]]

Reassigning to Jan-Ivar since Ben has gone back to school.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: adds telemetry for ice success rates. r=derf

Rebased and verified that new stat appears in about:telemetry. Carrying forward r+ from derf.

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa74b170d06

Comment 14

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/9aa74b170d06