Closed Bug 1208205 Opened 4 years ago Closed 4 years ago

[Metrics] Replace Settings Metrics checkboxes with 3 state radio button to match FTE

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
FxOS-S10 (30Oct)

People

(Reporter: thills, Assigned: rnicoletti)

References

Details

(Keywords: late-l10n)

Attachments

(3 files, 1 obsolete file)

We will need to do the following to be consistent with the FTE changes in Bug 1181295:
1.  Remove the telemetry checkbox in settings|Developer|developerHUD
2.  Replace the submit data checkbox in settings|B2g OS|submit data with the three state radio button.
3.  Working assumption is to disable the radio buttons for dogfooders with 'Enhanced' selected.  We will continue discussion with :sfoster on this and any changes would be in another bug.
Depends on: 1181295
Hi Evelyn, I’d appreciate your feedback on this patch. I’m especially interested in the unit test changes because I’m seeing two tests fail for reasons that I don’t understand. This is the output from one of the failing tests:

  1) [settings-test/unit/panels/improve_browser_os/panel_test.js] Improve browser os panel >  share performance data toggle the toggle should be enabled when not dog fooding:
     TypeError: panel.init(...) is undefined
      at (anonymous) (app://settings.gaiamobile.org/test/unit/panels/improve_browser_os/panel_test.js:68:7)

Thanks.
Assignee: nobody → rnicoletti
Status: NEW → ASSIGNED
Attachment #8668622 - Flags: feedback?(ehung)
Attached patch bug-1208205.patch (obsolete) — Splinter Review
Hi Jan, this is the patch that enables telemetry in the HUD based on the new "metrics level" setting.

The Gaia patch is attached in the previous comment.
Attachment #8668655 - Flags: review?(janx)
(In reply to Russ Nicoletti [:russn] from comment #3)
> Created attachment 8668655 [details] [diff] [review]
> bug-1208205.patch
> 
> Hi Jan, this is the patch that enables telemetry in the HUD based on the new
> "metrics level" setting.
> 
> The Gaia patch is attached in the previous comment.

Correction, comment 1 contains the Gaia patch.
Hi Tif, regarding the "Settings -> Improve FirefoxOS" part of the spec. It says "Basic is selected by default". Shouldn't the behavior be that whatever option was chosen during the FTE should be the default in "Settings -> Improve FirefoxOS"?
Flags: needinfo?(tshakespeare)
Whoops - you're totally right. Typo has been fixed and re-uploaded. Thanks Russ!
Flags: needinfo?(tshakespeare)
Jan, I've added a drive-by change to the patch to turn off telemetry debug logging by default. Eventually this will be controlled by a pref (bug 1193588).
Attachment #8668655 - Attachment is obsolete: true
Attachment #8668655 - Flags: review?(janx)
Attachment #8668731 - Flags: review?(janx)
Russ, thank you for writing this patch! I currently don't have access to a device, so I'll review your changes on Monday.
Hi Tif, I'd like your feedback on the styling of the updated "Improve B2g OS" page. Screenshots are here [1] or you can apply the "POC patch" from comment 1. Thanks.

[1] https://docs.google.com/a/mozilla.com/document/d/1d1ttpnSaQkmc8owIzXU43A1rIpot4U572srzTN0BRvg/edit?usp=sharing
Flags: needinfo?(tshakespeare)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Russn and I spoke over IRC - the only issue I noticed is that we need to address upgrade scenarios to ensure an option is always being selected. 

If previously, the user deselected sending data, then None should be chosen. If they were sending data, then we should select Basic since this is the new default. Thanks!

Eric - can you take a quick look and make sure it looks good from a visual stand point.
Flags: needinfo?(tshakespeare)
Attachment #8668622 - Flags: ui-review?(tshakespeare)
Attachment #8668622 - Flags: ui-review?(epang)
I've updated the patch (comment 1 attachment) to include the upgrade logic Tif described in comment 10.
QA Whiteboard: [COM=Telemetry]
Comment on attachment 8668731 [details] [diff] [review]
bug-1208205.patch

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

Your hud.js patch looks good and seems to work on device.

However, I think we will have to address some issues preventing Advanced Telemetry from working when the Developer HUD is off. E.g., the Developer HUD is initialized lazily, which means that at first boot, the code isn't even loaded until the HUD is enabled once. Enabling "Enhanced" data collection will probably not work if the HUD is not initialized. Solutions could be to initialize it eagerly (probably bad), or enable the HUD by default with no widgets and performance-light features (probably better).

Also, what will happen to the debug.performance_data.advanced_telemetry setting? It still seems to control at least some Advanced Telemetry features: https://github.com/mozilla-b2g/gaia/search?q=debug.performance_data.advanced_telemetry
Attachment #8668731 - Flags: review?(janx) → review+
A few clarifications on comment 12:
- The lazy DeveloperHUD issue happens on every boot with the HUD disabled, not just the first boot.
- My second suggestion is to enable the Developer HUD by default with performance-light metrics, in a way that doesn't show widgets (either the widgets are hidden, or disabled if Telemetry can report them anyway). However, maybe it makes sense to enable some HUD widgets by default? This would be the matter of a separate bug, but I imagine error / warning counters could be useful defaults on an Eng build of Firefox OS.
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

From a Visual standpoint this looks good to me!  Thanks Russ!
Attachment #8668622 - Flags: ui-review?(epang) → ui-review+
(In reply to Jan Keromnes [:janx] from comment #12)
> Comment on attachment 8668731 [details] [diff] [review]
> bug-1208205.patch
> 
> Review of attachment 8668731 [details] [diff] [review]:
> -----------------------------------------------------------------

> Also, what will happen to the debug.performance_data.advanced_telemetry
> setting? It still seems to control at least some Advanced Telemetry
> features:
> https://github.com/mozilla-b2g/gaia/search?q=debug.performance_data.
> advanced_telemetry

There are three patches involved in creating a three-way metrics selection to replace the FTU "submit data" checkbox and the Developer HUD "telemetry" checkbox: One is for the FTU [1], one is for Settings [2], and one is for the Developer HUD [3].

[1] https://github.com/mozilla-b2g/gaia/pull/32077 (bug 1181295)
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8668622 (this bug)
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8668731 (this bug)

There are three patches to address merging the Advanced Telemetry checkbox in the Developer HUD
More on comment 15, the reference to debug.performance_data.advanced_telemetry in 'build/config/common-settings.json' needs to be removed. That change is not yet part of any patch. I will add that change to the "third" patch mentioned in comment 15.
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Looks good and works! Thanks!
Attachment #8668622 - Flags: ui-review?(tshakespeare) → ui-review+
Attachment #8668622 - Flags: feedback?(ehung) → feedback?(gasolin)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Looks good to me, please use SettingsCache instead.
Attachment #8668622 - Flags: feedback?(gasolin) → feedback+
I have updated the patch to use SettingsCache and I made other changes based on your feedback. However, I am still getting unit test failures from test/unit/panels/improve_browser_os/panel_test.js

The error is "TypeError: panel.init(...) is undefined". Yet I now understand this is misleading because it seems to indicate the `panel.init` call cannot be made due to the `init` function being undefined; this is not true. `SettingsPanel.onInit` (defined in 'js/panels/improve_browser_os/panel.js') completes successfully. What I have noticed is the `then` block in the unit test cases is never executed. I added a comment to the PR indicating which code is not being executed. I can't find an explanation as to why these tests succeed on master but not with my patch.

Any advice/suggestions is appreciated.
Flags: needinfo?(gasolin)
I've look again and put more comments in github, generally you have to mockup all used api and libraries to make panel.init work

Here's a base work  panel_test sample for reference
https://gist.github.com/gasolin/9f3ba3e79dc86ee9c896
Flags: needinfo?(gasolin)
Attachment #8668622 - Attachment description: POC patch → git hub PR for settings app
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Hi Fred, I believe I've addressed all your feedback comments. I've added unit test cases for the new code and they are all passing.
Attachment #8668622 - Flags: review?(gasolin)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Thanks for the patch, I leave some comments on github, please address them and set review again.

BTW, the patch contain many new strings, have them being reviewed? If not please ni matej for copy editing as well.
Attachment #8668622 - Flags: review?(gasolin)
Keywords: late-l10n
@gasolin - the strings have been reviewed, not specifically Matej, as he was on PTO, but someone from the team looked them over.
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Hi Matej, can you review the strings that I've introduced as part of this PR? Thanks.
Attachment #8668622 - Flags: review?(matej)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Canceling, review, didn't see Tif's comment
Attachment #8668622 - Flags: review?(matej)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

I have updated the PR to address your review comments.
Attachment #8668622 - Flags: review?(gasolin)
May I know if any updates on this patch?
Flags: needinfo?(gasolin)
Sorry I got long review queues after the national holiday, will consume them accordingly
Flags: needinfo?(gasolin)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

The code looks good to me. Though I still have some concern about the strings. 

For example, the string `Send Basic report plus diagnostic information.` has two issues:

1. `Basic` should be in lowercase `basic` form
2. as in radio button description field, the end dot should not be there

It will be more efficient to do the strings right at once.
Flags: needinfo?(tshakespeare)
Attachment #8668622 - Flags: review?(gasolin)
Hey gasolin!

"B" is capitalized because in this instance the word basic is being used as a proper noun rather than an adjective. 

I believe the end dot you refer to is the period? Complete sentences should end with a period, ".". I'm sure you can point to instances where the period has been omitted but these are typos that need to be fixed.

Copywriters, UX, and PM have approved the strings.

Thanks and let me know if you have further questions!
Flags: needinfo?(tshakespeare)
Comment on attachment 8668622 [details] [review]
git hub PR for settings app

Thanks for clarify!
Attachment #8668622 - Flags: review+
(In reply to Jan Keromnes [:janx] from comment #12)
> Comment on attachment 8668731 [details] [diff] [review]
> bug-1208205.patch
> 
> Review of attachment 8668731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your hud.js patch looks good and seems to work on device.
> 
> However, I think we will have to address some issues preventing Advanced
> Telemetry from working when the Developer HUD is off. E.g., the Developer
> HUD is initialized lazily, which means that at first boot, the code isn't
> even loaded until the HUD is enabled once. Enabling "Enhanced" data
> collection will probably not work if the HUD is not initialized. Solutions
> could be to initialize it eagerly (probably bad), or enable the HUD by
> default with no widgets and performance-light features (probably better).
> 

I think what you're saying is that before this patch, the only way to enable telemetry was from the Developer HUD settings page when the Developer HUD is already enabled. But with the patch, enabling telemetry is decoupled from the Developer HUD. So that when setting the metrics level to 'enhanced' when the Developer HUD is off may not actually enable telemetry when the Developer HUD is turned on. Is that a fair summary?

If so, I have tested that scenario and all is well; when the Developer HUD starts, DeveloperHUD.init reads the metrics level setting via the SettingsListener and discovering the level to be 'enhanced' enables telemetry.

I want to be sure I am addressing the issue you raised. If I'm misunderstanding, please let me know.
Flags: needinfo?(janx)
Russ, the issue I was pointing out is that the hud.js file is loaded lazily[1]. This means that everything in there, including the telemetry code, won't be loaded until the Developer HUD has been activated at least once. This is problematic if you'd like to receive Telemetry events, but the user never activates the Developer HUD.

I mentioned two solutions:
- Init the Developer HUD right when Firefox OS starts, that way even if it's disabled, it will collect some metrics for Advanced Telemetry.
- Enable the Developer HUD by default (however, since a few probes are performance-heavy, maybe we can agree on a subset of metrics to enable by default with the HUD).

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#202
Flags: needinfo?(janx)
Note: This problem is out-of-scope for this bug, and doesn't impact the checkin-needed request. Maybe we should file a follow-up bug.
(In reply to Jan Keromnes [:janx] from comment #35)
> Note: This problem is out-of-scope for this bug, and doesn't impact the
> checkin-needed request. Maybe we should file a follow-up bug.

Bug 1215654
It looks like this check-in caused Gij 14 to fail:
TEST-UNEXPECTED-FAIL | apps/settings/test/marionette/tests/improve_settings_test.js | improve b2g improve page enable performance data

If a gaia PR test run is 8 days old, it's usually a good idea to rebase and do a new run before landing. I'll submit a PR that reverts this and see if it clears up the test.
https://hg.mozilla.org/mozilla-central/rev/4d0092c24ef6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
I'm reopening this bug because the gaia part was reverted due to a failing integration test -- comment 40.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8675465 [details] [review]
github PR, includes integration test fix

This PR resolves the failing integration test (couldn't update the original one as it was closed).
The treeherder link to the job corresponding to the PR is broken. This is the best I can come up with showing the successful completion of the job: [1] (revision 9ce2bce6fd2764d5cfcc7948f579b0ff500070c8)

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&author=russnicoletti@github.taskcluster.net
Master: https://github.com/mozilla-b2g/gaia/commit/496987319b5e4a62579b33e18d046b751050544f
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment #8675465 - Attachment description: [gaia] russnicoletti:bug-1208205 > mozilla-b2g:master → github PR, includes integration test fix
Depends on: 1216231
No longer blocks: 1219472
See Also: → 1219472
Verified@
Build ID               20151106013953
Gaia Revision          1fb7e4b7904d577a1859b91269a85b363ddc2836
Gaia Date              2015-11-06 02:30:49
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/f073181a927d63ae7abac86e47cc3872ccac7275
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151106.005858
Firmware Date          Fri Nov  6 00:59:06 UTC 2015
Bootloader             s1

Verify result:
- Telemetry checkbox has been removed in settings|Developer|developerHUD
- The submit data checkbox has been replaced in settings|Improve Foxfood with the three state radio button.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.