Closed Bug 1455724 Opened 2 years ago Closed Last year

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


(Core :: WebRTC, enhancement, P2)




Tracking Status
firefox63 --- fixed


(Reporter: ng, Assigned: ng, NeedInfo)



(2 files)

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.
Assignee: nobody → na-g
Here is a jsfiddle that can be used to manually test that the telemetry is being recorded as expected: .
Attachment #8970676 - Flags: review?(mfroman)
Attachment #8970676 - Flags: feedback?(chutten)
Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

Looks good to me.
Attachment #8970676 - Flags: review?(mfroman) → review+
Comment on attachment 8970676 [details]
Bug 1455724 - add telemetry for legacy PeerConnection getStats API usage

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:

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:
> +      -
> +    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.
Attachment #8970676 - Flags: feedback?(chutten)
Attachment #8972209 - Flags: feedback?(chutten)
Comment on attachment 8972209 [details]
Request for data collection - bug


    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?


    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?


    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+
Attachment #8972209 - Flags: feedback?(chutten) → feedback+
Flags: needinfo?(na-g)
Pushed by
add telemetry for legacy PeerConnection getStats API usage r=mjf
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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?
(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.

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

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.