1410101 - BackgroundHangMonitor should not check the toolkit.telemetry.enabled pref

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

The BackgroundHangMonitor uses the "toolkit.telemetry.enabled" pref to check if it needs to be enabled or not [1].

This should probably be changed to use |Telemetry::CanRecordExtended| instead.

On a related note, I'm not entirely sure how this code is excluding Beta users, since it's only checking the Telemetry pref and the client id.

[1] - http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp#561,580

Comment 1

[Chris H-C :chutten]

It's excluding Beta users using BHR_BETA_MOD. See https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1380081&attachment=8892562 from bug 1380081

Comment 2

[Chris H-C :chutten]

With bug 1410907 now landed, it should use Telemetry::CanRecordPrereleaseData() instead.

Comment 3

[Chris H-C :chutten]

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. You'll need to edit BackgroundHangMonitor.cpp to no longer check the "toolkit.telemetry.enabled" pref and to instead use Telemetry::CanRecordPrereleaseData() to determine whether or not it should be enabled.
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with mach build and test your change with mach test toolkit/components/telemetry/tests/
4) Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) Now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 4

[maera]

Hi,

Newbie here and a C++ student.  I will read all those docs listed up-thread, following which I expect to be in a a position to say: "I would like to help with this".  

cheers

maera

Comment 5

[Ashwin Patil]

Hey,

This would be my first bug for Mozilla. I've already built Firefox and working on the said file. I'll try to solve the problem and submit the patch once I am done.

Thanks
Ash

Comment 6

[Ashwin Patil]

Attachment: BackgroundHangMonitor.cpp

There was only one instance of the toolkit.telemetry.enabled in the BackgroundHangMonitor.cpp file and replaced with Telemetry::CanRecordPrereleaseData() which returns a boolean value.

I further build firefox and found it to be working. This is my first bug so please let me know if I goofed up somewhere in this.

Thanks
Ash

Comment 7

[Archit Jugran]

Attachment: I changed one line of code, called the function CanRecordPrereleaseData() which returns a bool value instead of using toolkit.telemetry.enabled pref

I have attached patch.I changed one line of code, called the function CanRecordPrereleaseData() which returns a bool value instead of using toolkit.telemetry.enabled pref . Please can you assign this bug to me and review and give feedback. :)
                                  Thanking you,
                                               Archit Jugran.
                                               (New to open source and mozilla)

Comment 8

[Ashwin Patil]

Attachment: Instance of the toolkit.telemetry.enabled in the BackgroundHangMonitor.cpp file and replaced with Telemetry::CanRecordPrereleaseData()

I uploaded the file directly the last time, I've attached a patch using hg export.

There was only one instance of the toolkit.telemetry.enabled in the BackgroundHangMonitor.cpp file and replaced with Telemetry::CanRecordPrereleaseData() which returns a boolean value.

Thanks 
Ash

Comment 9

[Chris H-C :chutten]

Oh wow! Figures all the activity on this bug would happen while I was off on holidays :S

I can only assign this bug to one person, so it goes to Ashwin Patil for pushing the first work-in-progress patch.

For Archit, if you're still ready to help, please comment on bug 1427765 which is about testing Telemetry::Accumulate. From there we can get it assigned to you.

For maera, if you're still ready to help, please comment on bug 1427766 which is about testing Telemetry::GetHistogramByName. From there we can get it assigned to you.

Comment 10

[Chris H-C :chutten]

Comment on attachment 8939088 [details] [diff] [review]
Instance of the toolkit.telemetry.enabled in the BackgroundHangMonitor.cpp file and replaced with Telemetry::CanRecordPrereleaseData()

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

This is exactly what we're aiming for, thank you!

Did you run the BHR tests with `mach test toolkit/components/backgroundhangmonitor/tests` to ensure the tests were written properly?

Can you write an informative commit message into your hg changeset and resubmit (upload the new patch with the new commit message, with me as reviewer)? The bug number is excellent context, but there are a few more pieces of information we need. The expected format for a commit message can be found here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 11

[Ashwin Patil]

Attachment: Bug 1410101 - Replaced "Preferences::GetBool("toolkit.telemetry.enabled")" with "Telemetry::CanRecordPrereleaseData()" to determine if it needs to be enabled or not; r=chutten

Hey Chris,

Edited the changeset to include informative commit message in the required format. I am little clueless about the patch format please check and tell me if anything else need to change.

Yes, I ran the BHR test and everything is okay. I further build firefox and its working as expected.

Thank you for helping.

Thanks
Ash

Comment 12

[Chris H-C :chutten]

Comment on attachment 8939621 [details] [diff] [review]
Bug 1410101 - Replaced "Preferences::GetBool("toolkit.telemetry.enabled")" with "Telemetry::CanRecordPrereleaseData()" to determine if it needs to be enabled or not; r=chutten

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

The patch format works as far as bugzilla is concerned, so I'm happy with it :)

Thank you for your contribution!

Comment 13

[Chris H-C :chutten]

I have marked your patch for inclusion at the next merge. It should be in the next Nightly or the one after that. Thanks!

Would you like some help finding the next thing to work on? Right now we're looking to expand our test coverage which will get you familiar with codecov.io and GTest (C++). We may also have other things to work on, but I'm not sure what those would be.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc473612a064
Replace "Preferences::GetBool("toolkit.telemetry.enabled")" with "Telemetry::CanRecordPrereleaseData()" in BackgroundHangMonitor.cpp to determine if it needs to be enabled or not. r=chutten

Comment 15

[Eliza Balazs [:ebalazs_]]

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

Comment 16

[Gerry Chang [:gchang]]

It's too late for 58. Mark 58 won't fix.