1455724 - Add telemetry for legacy callback based PeerConnection.getStats() API

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

We need to determine how much use there is in the wild of the legacy callback based PeerConnection.getStats() API, vs. promise based PeerConnection.getStats(), vs. mixed use of both, vs. use of neither. This information will be used to determine our path forward for the eventual removal of the legacy API.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Review commit: https://reviewboard.mozilla.org/r/239408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239408/

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Here is a jsfiddle that can be used to manually test that the telemetry is being recorded as expected: https://jsfiddle.net/j9vz2op5/ .

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239408/diff/1-2/

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e9eb1f5124c93650a7cede98f071be1204c5209

Comment 5

[Michael Froman [:mjf]]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

https://reviewboard.mozilla.org/r/239408/#review245902

Looks good to me.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

https://reviewboard.mozilla.org/r/239408/#review246216

On drive-by I see a couple of nits.

Also, this (as with all new or changed data collection) needs to undergo Data Collection Review: https://wiki.mozilla.org/Firefox/Data_Collection

It won't take long.

::: dom/media/PeerConnection.js:1
(Diff revision 2)
> +

Seems like unintentional whitespace here.

::: dom/media/PeerConnection.js:48
(Diff revision 2)
>  
> +const TELEMETRY_PC_CONNECTED = "webrtc.peerconnection.connected";
> +const TELEMETRY_PC_CALLBACK_GETSTATS = "webrtc.peerconnection.legacy_callback_stats_used";
> +const TELEMETRY_PC_PROMISE_GETSTATS = "webrtc.peerconnection.promise_stats_used";
> +const TELEMETRY_PC_PROMISE_AND_CALLBACK_GETSTATS = "webrtc.peerconnection.promise_and_callback_stats_used";
> +const TELEMETRY_NICER_STUN_RETRANSMITS = "webrtc.nicer.stun_retransmits";

Is this constant used?

::: toolkit/components/telemetry/Scalars.yaml:641
(Diff revision 2)
> +    notification_emails:
> +      - [e-mail]
> +    release_channel_collection: opt-in
> +    record_in_processes:
> +      - 'main'
> +      - 'content'

To match the rest of the file you should probably have a line of whitespace between the definitions.

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Request for data collection - bug 1455724.md

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239408/diff/2-3/

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239408/diff/3-4/

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239408/diff/4-5/

Comment 11

[Chris H-C :chutten]

Comment on attachment 8972209 [details]
Request for data collection - bug 1455724.md

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Standard Telemetry mechanisms.

    Is there a control mechanism that allows the user to turn the data collection on and off? 

Standard Telemetry mechanisms.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A, not permanent.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 1, technical.

    Is the data collection request for default-on or default-off?

Default-on

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No, just counts.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :ng, please file a follow-up for evaluating whether to renew the data. (All else fails you should be alerted at the alert_emails addresses well in advance of your probes' expiry)

---
Result: datareview+

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d94aa8ef3c68
add telemetry for legacy PeerConnection getStats API usage r=mjf

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d94aa8ef3c68

Comment 14

[Ciprian Georgiu, Desktop QA]

Hi Nico,

I was trying to manually very this bug using the jsfiddle from comment 2, to see if the telemetry is being recorded as expected in a fixed build.

I'm not sure, however, what scalars should appear in about:telemetry->Scalars->content, or what values they should have.

Could you please tell me a bit more about the expected result here?

Comment 15

[Ciprian Georgiu, Desktop QA]

Forgot to flag you. :)

Comment 16

[Ciprian Georgiu, Desktop QA]

(In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from comment #15)
> Forgot to flag you. :)

Oh, you already have a ni?. Missed that.

Comment 17

[Ciprian Georgiu, Desktop QA]

Due to lack of additional info regarding the expected behavior, I will remove the qe-verify+ flag.