Closed Bug 1396811 Opened 7 years ago Closed 7 years ago

Add new telemetry probe collecting the current theme

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed, firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(1 file)

Bug 1396811 - Add new telemetry probe collecting the current theme;
59 bytes, text/x-review-board-request
chutten
: review+
liuche
: review+
ochameau
: review+
lizzard
: approval-mozilla-beta+
lizzard
: approval-mozilla-release-
Details 
This is a follow up for bug 1393479.

DevTools need to collect data about theme usage (specifically we need to know how many users use the Firebug theme).

Honza
So, we need to collect data about DevTools theme usage (light, dark, firebug). It should only happen for users that open the Toolbox at least once. I.e. only for those who actually use DevTools.

Should I do a log including the current theme every time the user opens the Toolbox?
(the current theme is stored in a pref)

I am thinking about something as follow...

// When the Toolbox is opened...
var Telemetry = require("devtools/client/shared/telemetry");
let telemetry = new Telemetry();
telemetry.toolboxOpened(currentTheme);

// In Telemetry object...
toolboxOpened: function (currentTheme) {
  let charts = this._histograms['currentTheme'];

  this.logScalar('devtools.currentTheme', currentTheme);
}

Does that make sense?

Honza
@jryans: pleas see comment #1

Or please, let me know who else I should ask.

Honza
Flags: needinfo?(jryans)
Seems great to me, but :chutten says:
> If you wanted was the number of times devtools was opened with each theme, we're talking a keyed count scalar, not a string scalar
> What you're getting right now is "Did the user open the devtools this subsession? And if so, what was the most recent theme they opened it with?"

So if we switched to that, it would let us answer more questions (like "Do people stick with the same theme all the time, or switch themes?"), which seems like a useful thing to know.  So that's my only suggestion.  
As always, it depends on what questions you hope to answer with your data.

If all you want is "Are all of these themes being used?" then this approach will get you that (so long as theme names are shorter than 50 characters).

If what you want is "What proportion of users use which themes?" This will get you approximately that, assuming most users are unlikely to change themes frequently.

If you want something proportional relative to the number of times the toolbox is opened, like "Of all the toolbox opens on <DATE> what %ge were on theme <THEME>?" then you'll want a keyed scalar and to use Telemetry.scalarAdd instead of scalarSet. (Probably not a question you're interested in asking, but who am I to say :) )
Summary: Add new telemetry prob collecting the current theme → Add new telemetry probe collecting the current theme
(In reply to Chris H-C :chutten from comment #4)
> If what you want is "What proportion of users use which themes?" This will
> get you approximately that, assuming most users are unlikely to change
> themes frequently.
Yes

So, for example, the answer can be:
light: 50% of users use the Light theme (yeah, most user since it's the default one)
dark: 20% of users use the Dark theme
firebug: 30% users use the Firebug theme

> Do people stick with the same theme all the time, or switch themes?
Agreed, seems also useful.

> If you wanted was the number of times devtools was opened with each theme,
> we're talking a keyed count scalar, not a string scalar
So, should I use `keyedScalarAdd`?

Somethings like as follows?

function onDevToolsToolboxOpened() {
  let currentTheme = Services.prefs.setStringPref("devtools.theme");
  Services.telemetry.keyedScalarAdd(
    "devtools.toolbox.opened.current.theme.count", currentTheme, 1);
}

Honza
Flags: needinfo?(chutten)
That would indeed be correct except I think you want getStringPref, not setStringPref... and you'll have to prepare for bikeshedding on the scalar name :)

But only if you want to. The existing solution is fine and straightforward if the question you want answered is "What proportion of users use which themes?"
Flags: needinfo?(chutten)
I am not up to date on the latest Telemetry APIs, so I defer to :chutten here.

From a quick scan, the `keyedScalarAdd` approach seems reasonable to me.
Flags: needinfo?(jryans)
@Chris: this page [1] says I should ask :bsmedberg for feedback (I guess it should be updated) for adding new data collection and this page [2] says your name. So, it should be enough to ask you for review, right? Or should I ask yet someone else?

Honza

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
[2] https://wiki.mozilla.org/Modules/Toolkit#Telemetry
Flags: needinfo?(chutten)
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

@liuche: we are adding a new data collection in this patch. Does it sound ok?

Honza
Attachment #8905016 - Flags: feedback?(liuche)
Looks like you found the https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval page that lists the current Data Peer list. I've updated the MDN article to not reference :bsmedberg and instead hopefully guide people to the Data Collection Review page where they can peruse the up-to-date list of Data Peers.

I'm an okay person to ask for review to make sure your data collection will collect what you expect, that there isn't a better way to get the information you want, and that you haven't found one of our lurking footguns. I'll give the patch a look-see and let you know if anything comes up wrt your use of Telemetry.
Flags: needinfo?(chutten)
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review181820

This is missing a few pieces.

1) The scalar you're recording to must be defined in toolkit/components/telemetry/Scalars.yaml
2) Scalars have a category (or group) which is underscore_delimited and a name which is dot.delimited (which allows nesting of like scalars together, and allows us an opportunity to bikeshed). Currently devtools scalars are a complete mess with overly-verbose and occasionally-duplicated categories and unhelpful names. For this I'd recommend a "devtools" category and a "current.theme" name.
3) Once you have the definition named and put in correctly, there are additional pieces that liuche will be able to help you with: expiration, notification emails, record_in_processes, and release_channel_collection.

::: devtools/client/framework/toolbox.js:15
(Diff revision 1)
>  const SPLITCONSOLE_ENABLED_PREF = "devtools.toolbox.splitconsoleEnabled";
>  const SPLITCONSOLE_HEIGHT_PREF = "devtools.toolbox.splitconsoleHeight";
>  const DISABLE_AUTOHIDE_PREF = "ui.popup.disable_autohide";
>  const HOST_HISTOGRAM = "DEVTOOLS_TOOLBOX_HOST";
>  const SCREENSIZE_HISTOGRAM = "DEVTOOLS_SCREEN_RESOLUTION_ENUMERATED_PER_USER";
> +const CURRENT_THEME_SCALAR = "DEVTOOLS_CURRENT_THEME";

scalar categories and names are lowercase
Attachment #8905016 - Flags: review?(chutten) → review-
(In reply to Chris H-C :chutten from comment #12)
> 3) Once you have the definition named and put in correctly, there are
> additional pieces that liuche will be able to help you with: expiration,
> notification emails, record_in_processes, and release_channel_collection.
Flags: needinfo?(liuche)
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review182318

To manually check that this collection is working you can open about:telemetry in your Firefox and check the Keyed Scalars section (about:telemetry#keyed-scalars-tab). Open and close the toolbox with different themes and you should only have to refresh about:telemetry for it to show you data.

::: toolkit/components/telemetry/Scalars.yaml:779
(Diff revision 2)
>      release_channel_collection: opt-out
>      record_in_processes:
>        - 'main'
>  
> +devtools:
> +  current.theme:

names are underscore_case

::: toolkit/components/telemetry/Scalars.yaml:784
(Diff revision 2)
> +  current.theme:
> +    bug_numbers:
> +      - 1396811
> +    description: >
> +      What proportion of users use which DevTools themes?
> +    expires: never

Generally speaking, for never-expiring probes you'll need automated tests to ensure the probe continues to work, a human's address on the notification_emails field, and some additional scrutiny in the Data Collection Review.

Once again I defer to a Data Peer like :liuche to have the final word one what is and is not necessary.

A notable fact is that this collects strings (since it is keyed), so depending on where the strings come from it could be a little sensitive.

::: toolkit/components/telemetry/Scalars.yaml:785
(Diff revision 2)
> +    bug_numbers:
> +      - 1396811
> +    description: >
> +      What proportion of users use which DevTools themes?
> +    expires: never
> +    kind: uint

Speaking of which, you'll need a keyed: true field in this definition as well.
Attachment #8905016 - Flags: review?(chutten) → review-
(In reply to Chris H-C :chutten from comment #15)
> To manually check that this collection is working you can open
> about:telemetry in your Firefox and check the Keyed Scalars section
> (about:telemetry#keyed-scalars-tab). Open and close the toolbox with
> different themes and you should only have to refresh about:telemetry for it
> to show you data.
> 
> names are underscore_case
Fixed

> Generally speaking, for never-expiring probes you'll need automated tests to
> ensure the probe continues to work, a human's address on the
> notification_emails field, and some additional scrutiny in the Data
> Collection Review.
> 
> Once again I defer to a Data Peer like :liuche to have the final word one
> what is and is not necessary.
> 
> A notable fact is that this collects strings (since it is keyed), so
> depending on where the strings come from it could be a little sensitive.
The string come from preferences ("devtools.theme"). The possible values are:
- light
- dark
- firebug

> Speaking of which, you'll need a keyed: true field in this definition as
> well.
Done

Honza
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review182410

One nit which I really should have caught earlier, sorry!

::: toolkit/components/telemetry/Scalars.yaml:783
(Diff revision 3)
> +devtools:
> +  current_theme:
> +    bug_numbers:
> +      - 1396811
> +    description: >
> +      What proportion of users use which DevTools themes?

Annnnd this is where I get nitpicky and apologetic for not being nitpicky beforehand.

This probe is more accurately "Number of times DevTools was opened, by theme". It doesn't know anything about users or proportions. That comes from analysis much later in the pipeline.
Attachment #8905016 - Flags: review?(chutten) → review+
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review182500

r- on data review, this should be specific about what is being collected.

For example, in Histograms.json, the descriptions cover exactly what is being collected, and under what circumstances they're being collected - this description should do the same.

::: toolkit/components/telemetry/Scalars.yaml:783
(Diff revision 3)
> +devtools:
> +  current_theme:
> +    bug_numbers:
> +      - 1396811
> +    description: >
> +      What proportion of users use which DevTools themes?

Agreed, from a data peer perspective (and here I'm only reviewing the documentation, and defer to chutten for the usage of this Scalar), this needs to be very specific what is being collected.
Attachment #8905016 - Flags: review-
Flags: needinfo?(liuche)
Thanks for the review!

The description is updated.

Honza
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review182870

Thanks for the quick turnaround. data-review+ only

::: toolkit/components/telemetry/Scalars.yaml:783
(Diff revision 5)
> +devtools:
> +  current_theme:
> +    bug_numbers:
> +      - 1396811
> +    description: >
> +      Number of times DevTools was opened, by theme.

Nit: keyed by theme
Attachment #8905016 - Flags: review?(liuche) → review+
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review183326

::: devtools/client/framework/toolbox.js:658
(Diff revision 6)
>      this._telemetry.log(HOST_HISTOGRAM, this._getTelemetryHostId());
> +
> +    // Log current theme. The question we want to answer is:
> +    // "What proportion of users use which themes?"
> +    let currentTheme = Services.prefs.getCharPref("devtools.theme");
> +    this._telemetry.logKeyedScalar(CURRENT_THEME_SCALAR, currentTheme, 1);

I don't know exactly how telemetry's keyed scalar work,
but, this is going to increment the value for whatever theme is used, each time a toolbox is opened.

With that behavior, I imagine people that toggle toolbox a lot are going to make this probe confusing.

Don't you want to bump that only once?!

Then, I imagine somewhere in:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js
Would be better.
Now that devtools codebase is lazy loaded, you know that this module is loaded whenever the user triggers any devtools entry point. Now that may be gcli, or some command line argument. So it isn't necessary displaying a toolbox.
Otherwise, you may keep the code in toolbox.js but make it so that you log the theme only once.
Attachment #8905016 - Flags: review?(poirot.alex)
I don't think it will be confusing, because we'll be able to see the number of times each session opened each theme, so could slice on either >1 or on the numbers themselves, to see which theme is opened more as well as which themes people open per-session.  (Also, if someone opens the light theme 50 times, and the firebug theme once, I feel like we would be interested in that data, and I can't think of a better way to record that…)
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

https://reviewboard.mozilla.org/r/176846/#review183338

Ok. I wasn't sure what you could do from telemetry's dashboard.
Looks like you can do what you want if you can filter with ">1 per session".
Looks good then.
Attachment #8905016 - Flags: review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b86983c73434
Add new telemetry probe collecting the current theme; r=chutten,liuche,ochameau
https://hg.mozilla.org/mozilla-central/rev/b86983c73434
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: No impact, the patch introduces telemetry probe
[Is this code covered by automated tests?]: -
[Has the fix been verified in Nightly?]: Just landed in Nightly and it would be great to uplift now before end of the cycle (to get data asap)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: it's just new telemetry probe.
[String changes made/needed]: n/a
Attachment #8905016 - Flags: approval-mozilla-release?
Attachment #8905016 - Flags: approval-mozilla-beta?
(In reply to Jan Honza Odvarko [:Honza] from comment #31)
> [Why is the change risky/not risky?]: it's just new telemetry probe.
To confirm, this change appears very low risk. It's a new scalar probe, with a line of code changed in DevTools. This code reads the value of a pref, and logs it in the new telemetry probe.
This code will only run when DevTools is open.
Our goal is to retrieve data about the usage of DevTools themes out there in the wild. Which is why we'd like to uplift this new probe all the way to release.
Just to be clear, you are looking to get this on 56 release? 
It seems unlikely we will do another 55 dot release at this point.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> Just to be clear, you are looking to get this on 56 release? 
Yes

Honza
Comment on attachment 8905016 [details]
Bug 1396811 - Add new telemetry probe collecting the current theme;

Adding a pref to telemetry, limited to dev tools users. 
OK to uplift this for beta 12.
Attachment #8905016 - Flags: approval-mozilla-release?
Attachment #8905016 - Flags: approval-mozilla-release-
Attachment #8905016 - Flags: approval-mozilla-beta?
Attachment #8905016 - Flags: approval-mozilla-beta+
Assignee: nobody → odvarko
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> Comment on attachment 8905016 [details]
> Bug 1396811 - Add new telemetry probe collecting the current theme;
> 
> Adding a pref to telemetry, limited to dev tools users. 
> OK to uplift this for beta 12.
Thanks!

When we should expect to get some data from 56 release?
I am assuming we'll see results here [1]

Honza

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-09-18&keys=__none__!__none__!__none__!__none__&max_channel_version=release%252F56&measure=SCALARS_DEVTOOLS.CURRENT_THEME&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-09-12&table=0&trim=1&use_submission_date=0
Flags: needinfo?(lhenry)
There are so few results (we're seeing on the order of tens of samples) that the data is falling below the "Sanitize" threshold for display on telemetry.mozilla.org.

If you open the Advanced Settings at the bottom of the page and select "Don't Sanitize" you can get this: https://mzl.la/2fU0Gpw

(( to get the short links for sharing, click on the link icon at the top-right of the page ))
Flags: needinfo?(lhenry)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: