Closed Bug 1241508 Opened 4 years ago Closed 4 years ago

WebRTC Telemetry should only be recorded with extended Telemetry on

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: gfritzsche, Assigned: ma.steiman, Mentored)

Details

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

Attachments

(1 file)

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.
Whiteboard: [measurement:client] [lang=c++] → [measurement:client] [lang=c++] [good first bug]
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)
I'd love to submit a patch for this if that's ok
Flags: needinfo?(gfritzsche)
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+
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
Attachment #8710652 - Flags: review?(gfritzsche)
https://hg.mozilla.org/mozilla-central/rev/a45569eb51d7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.