WebRTC Telemetry should only be recorded with extended Telemetry on

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: Tyler Steiman, Mentored)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [measurement:client] [lang=c++] [good first bug])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This recording function doesn't check for mCanRecordExtend:
https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/toolkit/components/telemetry/Telemetry.cpp#3651

This means it is always recording, even when extended Telemetry is off and we never use the data.
(Reporter)

Updated

2 years ago
Whiteboard: [measurement:client] [lang=c++] → [measurement:client] [lang=c++] [good first bug]
(Reporter)

Comment 1

2 years ago
Nils, is there any reason for this to always record?
Flags: needinfo?(drno)
Besides that I don't know what extended Telemetry means not, no :-)
My best guess is that I copied the if condition which checks for |sTelemetry| from some other function (which on purpose does not check for |mCanRecordExtended|).

Happy to provide a patch, or should I leave it for someone else as a first bug?
Flags: needinfo?(drno)
(Assignee)

Comment 3

2 years ago
I'd love to submit a patch for this if that's ok
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 4

2 years ago
Created attachment 8710652 [details] [diff] [review]
bug1241508_mCanRecordExtended_check.diff

Does this work?
Assignee: nobody → ma.steiman
Status: NEW → ASSIGNED
Attachment #8710652 - Flags: review?(gfritzsche)
Comment on attachment 8710652 [details] [diff] [review]
bug1241508_mCanRecordExtended_check.diff

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

LGTM
Attachment #8710652 - Flags: review+
(Assignee)

Comment 6

2 years ago
Can I go ahead and put checkin-needed or should we wait for Georg?
Flags: needinfo?(gfritzsche) → needinfo?(drno)
As I'm the original author of these lines and it's just one line fix, I'm pretty sure Georg doesn't mind if we set checkin-needed :-)
Flags: needinfo?(drno)
Keywords: checkin-needed

Comment 8

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a45569eb51d7
Keywords: checkin-needed
(Reporter)

Updated

2 years ago
Attachment #8710652 - Flags: review?(gfritzsche)

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a45569eb51d7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.