Closed Bug 1215654 Opened 9 years ago Closed 9 years ago

[Metrics] Metrics may not be recorded when level set to enhanced

Categories

(Firefox OS Graveyard :: Metrics, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.5?)

VERIFIED FIXED
blocking-b2g 2.5?

People

(Reporter: rnicoletti, Assigned: thills)

References

Details

Attachments

(3 files)

Before the telemetry checkbox in the DeveloperHUD settings was replaced with three-way radio buttons in 'Improve Firefox OS' (bug 1208205), the Developer HUD was always active before telemetry was enabled (the 'telemetry' checkbox was enabled only when Developer Tools were enabled). In this case, the Developer HUD immediately recognized telemetry was enabled and began to collect telemetry metrics.

With bug 1208205, enabling telemetry is decoupled from enabling the Developer HUD. Therefore, if the Developer HUD is not enabled with the metrics level is set to "Enhanced", telemetry metrics will not be collected until the Developer HUD is enabled.

Some possible 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).
- Init the Developer HUD when the metric level is set to "Enhanced"
Jan, can you explain the difference between options 1 and 2 above with respect to performance? Isn't it the case that whether the Developer HUD is initialized or enabled, all probes corresponding to enabled Developer HUD metrics will be active? For example, if uss memory is checked and Developer HUD is initialized -- but not enabled, the uss probe will still be active?
Flags: needinfo?(janx)
Wouldn't we also have to make sure that 'hide all HUD Elements' is checked?
QA Whiteboard: [COM=Telemetry]
Russ, I think the third option you listed is what we'll want.

The first option (which I suggested) might not actually be valid, because you can't actually have Advanced Telemetry working while the HUD is completely off (that was my premise for option 1).

As we discussed with Tamara on IRC, we'll probably want to enable the HUD by default if "Enhanced data collection" is enabled. However, in that case, all of the widgets should be unchecked (i.e. not with "Hide all HUD elements", but with not a single metric enabled). That way users won't be confused by random widgets showing up on the screen (unless they explicitly ask for them), but Advanced Telemetry will still be able to report important data like errors/warnings/reflows and performance entries.

However, before landing such a change, we need to make sure that the performance impact of enabling the HUD by default won't degrade the user experience. The devtools actors being installed into every frame are now lighter than they used to be, and with no constant memory polling or eventloop-lag-watching in progress, the perf hit should be close to invisible (at least it seems that way on my Z3C). We should make sure that it's close to invisible on lower end devices as well (e.g. the Flame).
Flags: needinfo?(janx)
(In reply to Russ Nicoletti [:russn] from comment #1)
> Isn't it the case that whether the Developer HUD is
> initialized or enabled, all probes corresponding to enabled Developer HUD
> metrics will be active? For example, if uss memory is checked and Developer
> HUD is initialized -- but not enabled, the uss probe will still be active?

That is what I thought and maybe told you, but it is not actually the case. When the user disables the HUD, the "devtools.overlay" setting is set back to "false", causing a call to "developerHUD.uninit()"[1], which then starts untracking all the frames[2], by stopping all watchers and finally destroying the Target object[3].

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#208
[2] https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools/hud.js#122
[3] https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools/hud.js#157
[Blocking Requested - why for this release]:
Metrics will not be turned on without enabling the HUD.
blocking-b2g: --- → 2.5?
Component: Gaia::First Time Experience → Metrics
Attachment #8677112 - Flags: feedback?(rnicoletti)
Comment on attachment 8677112 [details] [review]
[gaia] tamarahills:bugfix/1215654-ftu-enable-hud-when-enhanced-metrics-chosen > mozilla-b2g:master

I left a comment in the PR.
Attachment #8677112 - Flags: feedback?(rnicoletti) → feedback+
Hi Tamara, this is a patch for the settings part of this bug. I haven't tested it yet. I appreciate any feedback you have for me.
Attachment #8677227 - Flags: feedback?(thills)
Comment on attachment 8677112 [details] [review]
[gaia] tamarahills:bugfix/1215654-ftu-enable-hud-when-enhanced-metrics-chosen > mozilla-b2g:master

Hi Sam,

This is a patch to turn on the HUD when the user chooses 'enhanced' metrics.  This will enable the Gecko portion of the HUD to start collecting metrics.  I've also hidden the HUD elements from the user.

Thanks,

tamara
Attachment #8677112 - Flags: review?(sfoster)
Assignee: rnicoletti → thills
Status: NEW → ASSIGNED
Comment on attachment 8677227 [details] [diff] [review]
bug-1215654-settings.patch

Hi Russ,

Looks good.  Assuming you are adding the tests next.  

One comment is I think on line 130 you have |if (level === 'enhanced')|.  Shouldn't that be 'Enhanced'?

So, if the Hud was turned on, you are just returning.  Just to talk this through, if the hud is on we are leaving the widgets alone, right?  I believe this is the right thing to do.

Thanks,
-tamara
Attachment #8677227 - Flags: feedback?(thills) → feedback+
Comment on attachment 8677112 [details] [review]
[gaia] tamarahills:bugfix/1215654-ftu-enable-hud-when-enhanced-metrics-chosen > mozilla-b2g:master

Left a comment in the PR about the unit test. r+ with that change (and assuming the test passes!)
Attachment #8677112 - Flags: review?(sfoster) → review+
Comment on attachment 8678310 [details] [review]
[gaia] russnicoletti:bug-1215654 > mozilla-b2g:master

Hi Fred, this bug is a follow-up to bug 1208205, where the 'telemetry' checkbox on the Developer HUD settings page was removed in favor of a three-state radio button on the 'Improve Firefox OS' page (and in the FTU flow). 

Prior to bug 1208205, enabling telemetry was only possible when Developer Tools was enabled. This was convenient because telemetry requires Developer Tools to be enabled. After bug 1208205, enabling telemetry is decoupled from enabling Developer Tools. 

This bug restores the connection between enabling telemetry and enabling Developer Tools.
Attachment #8678310 - Flags: review?(gasolin)
Comment on attachment 8678310 [details] [review]
[gaia] russnicoletti:bug-1215654 > mozilla-b2g:master

Thanks for the patch. Some comments in github.

The concern is the show/hide HUD widgets does not related to this function. I'd suggest we only control 'devtools.overlay' here, and change the default value of 'hud.hide' to `true` in common-settings.js. 

If developer want to show the HUD widget, they could check it manually in developer menu.


When switching between Basic and None, the adb logcat shows

[JavaScript Error: "TypeError: target.actor is null" {file: "chrome://b2g/content/devtools/hud.js" line: 494}]

This error happens in gecko so it might be another issue.
Attachment #8678310 - Flags: review?(gasolin)
Comment on attachment 8678310 [details] [review]
[gaia] russnicoletti:bug-1215654 > mozilla-b2g:master

Fred, thanks for the suggestion of hiding the HUD widgets by default. I like how that removes some of the coupling between the Gaia settings and the gecko Developer HUD. I have updated the PR to make this change.

Regarding the error you saw when changing the level from Basic to None, I'm not seeing that error. Are you seeing it consistently?
Attachment #8678310 - Flags: review?(gasolin)
Comment on attachment 8678310 [details] [review]
[gaia] russnicoletti:bug-1215654 > mozilla-b2g:master

Thanks for update, please address comments in github before land.


The console error might be caused by I haven't update gecko for a while :p
Attachment #8678310 - Flags: review?(gasolin) → review+
Master: https://github.com/mozilla-b2g/gaia/commit/70faabfe5b08bc8b4043cbe1e4d2d180de3a24f2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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:
Metrics can be recorded when level set to enhanced
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.