1193588 - [Metrics] Add pref for HUD telemetry logging

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Russ Nicoletti [:russn]]

Currently, the HUD backend will log telemetry events based on variable in the js. This is not ideal, as some of the metrics generate many events which will create a lot of noise in the log that most developers will not want to see.

Developers should be able to enable and disable HUD telemetry logging using a pref.

Comment 1

[Russ Nicoletti [:russn]]

Attachment: telemetry_logging.patch

Jan, I have created a new pref for "enhanced metrics" logging. I considered using the existing "hud.logging" but I decided HUD logging and telemetry logging should be separate. Thanks in advance for your feedback!

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8681540 [details] [diff] [review]
telemetry_logging.patch

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

Feedback+ with nits. Please don't create a pref associated to the setting (since we only need the latter).

Are you planning to expose this in the Settings app? If so, where? I feel like the Developer HUD panel is getting crowded with checkboxes already.

::: b2g/app/b2g.js
@@ +1152,5 @@
>  pref("b2g.multiscreen.chrome_remote_url", "chrome://b2g/content/shell_remote.html");
>  pref("b2g.multiscreen.system_remote_url", "index_remote.html");
>  
> +// Enhanced metrics (telemetry)
> +pref("metrics.level.enhanced.logging": "false");

No need to create an associated pref, we just need the setting here.

::: b2g/chrome/content/devtools/hud.js
@@ +111,5 @@
>      SettingsListener.observe('metrics.selectedMetrics.level', "", level => {
>        this._telemetry = (level === 'Enhanced');
>      });
> +
> +    SettingsListener.observe('metrics.level.enhanced.logging', this._telemetryDebug, enabled => {

Please don't use unnecessarily long setting names, unless you're certain there will be other settings also starting with "metrics.level.enhanced.*" or even "metrics.level.*".

On a side-note, "metrics.selectedMetrics.level" I also find unnecessarily long, because there are no other settings called "metrics.selectedMetrics.*" (i.e. "metrics.level" would have worked fine). No point in fixing this though, because the code is already landed (and I r+'d the gecko patch).

If you're unsure about "metrics.logging", may I suggest "metrics.enhanced.logging"?

::: b2g/chrome/content/settings.js
@@ +667,5 @@
>    // Restart required
>    'layers.async-pan-zoom.enabled': false,
>  #endif
>    'layout.display-list.dump': false,
> +  'metrics.level.enhanced.logging': false,

As above, no need to map the setting to a pref.

Comment 3

[Russ Nicoletti [:russn]]

Because it will be rarely used, I wasn't planning on exposing it in the Settings app (although I think the  |Developer| screen, in the |Debug| section would be appropriate). The main benefit I see of exposing it would be so developers know it exists. What's your opinion about that tradeoff of adding a checkbox to publicize the setting (yet another checkbox) vs. not cluttering the Developer settings and essentially having a secret setting? 

About the setting name, after discussing with Tamara, we want this setting to control only HUD telemetry logging not HUD *and* gaia telemetry logging. In that case, "hud.telemetry.logging" would be consistent with the existing HUD settings. What do you think?

Comment 4

[Jan Keromnes [:janx]]

(Very busy at the moment, will look at this in a few days -- sorry for the delay!)

Comment 5

[Russ Nicoletti [:russn]]

I'd like to get your opinion regarding comment 3.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Russ Nicoletti [:russn] from comment #3)
> Because it will be rarely used, I wasn't planning on exposing it in the
> Settings app (although I think the  |Developer| screen, in the |Debug|
> section would be appropriate). The main benefit I see of exposing it would
> be so developers know it exists. What's your opinion about that tradeoff of
> adding a checkbox to publicize the setting (yet another checkbox) vs. not
> cluttering the Developer settings and essentially having a secret setting? 

I don't have a strong opinion about this.  If you think a visible Developer setting would be convenient to have, maybe you should add one.  It seems like you don't see any value, so I'd say leave it out.  You can still control it from WebIDE, and can always expose in the UI later if desired.

> About the setting name, after discussing with Tamara, we want this setting
> to control only HUD telemetry logging not HUD *and* gaia telemetry logging.
> In that case, "hud.telemetry.logging" would be consistent with the existing
> HUD settings. What do you think?

Sounds fine, but :janx likely will have a stronger opinion than me.

Comment 7

[Jan Keromnes [:janx]]

Thank you Ryan for helping out here!

(In reply to Russ Nicoletti [:russn] from comment #3)
> Because it will be rarely used, I wasn't planning on exposing it in the
> Settings app (although I think the  |Developer| screen, in the |Debug|
> section would be appropriate). The main benefit I see of exposing it would
> be so developers know it exists. What's your opinion about that tradeoff of
> adding a checkbox to publicize the setting (yet another checkbox) vs. not
> cluttering the Developer settings and essentially having a secret setting?

Not exposing it in the Settings app sounds good to me (and your suggested location would work by the way).

Are you just creating the setting to turn logging off by default now, but leave the possibility to re-enable it later? (E.g. by the user with help of WebIDE, or changing its default value to "true" in debug builds / a future release if necessary). In that case it's fine to hide it by default, and as Ryan said, it's always possible to expose it later should the need arise.

> About the setting name, after discussing with Tamara, we want this setting
> to control only HUD telemetry logging not HUD *and* gaia telemetry logging.
> In that case, "hud.telemetry.logging" would be consistent with the existing
> HUD settings. What do you think?

Perfect! Thank you for that, I'll r+ your patch with that setting name and nits addressed.

Again, sorry for the week-long lag. This shouldn't happen in normal conditions.

Comment 8

[Russ Nicoletti [:russn]]

Attachment: telemetry_logging.patch

Hi Jan, attached is the updated patch for your review.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8696009 [details] [diff] [review]
telemetry_logging.patch

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

R+ with one nit. Thanks for addressing my earlier comments!

::: b2g/chrome/content/devtools/hud.js
@@ +107,5 @@
>      SettingsListener.observe('hud.logging', this._logging, enabled => {
>        this._logging = enabled;
>      });
>  
> +    SettingsListener.observe('hud.telemetry.logging', this._telemetryDebug, enabled => {

Nit: `this._telemetryDebug` is undefined here, you probably meant just `_telemetryDebug`.

Comment 10

[Russ Nicoletti [:russn]]

Attachment: telemetry_logging.patch

Updated patch addressing final nit.

Comment 11

[Russ Nicoletti [:russn]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a761950444d3

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/5e54bf961092

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e54bf961092