Closed Bug 1378863 Opened 7 years ago Closed 7 years ago

Add telemetry probe to addon ui shim to track entry points and addon installations

Categories

(DevTools :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file)

For now, we never tracked devtools entry points.
We don't know from where users get into devtools:
* menus
* key shotcuts
* right click, inspect element
* command line
* ...

Also we should track add-on installation to ensure we don't loose developers and also see how many people are mislead into devtools shortcuts.
Morphing this bug to only track entry point and ignore addon installation for now.
Assignee: nobody → poirot.alex
Depends on: 1359855
No longer depends on: dt-addon-uishim
Note that for testing telemetry patches, artifacts builds won't work. You have to build everything...
Comment on attachment 8884011 [details]
Bug 1378863 - Add telemetry probe to track DevTools entry points.  datareview=francois

https://reviewboard.mozilla.org/r/154964/#review165882

The code seems fine, but according to the documentation (https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#changing-a-histogram) changing an existing histogram should not be done lightly. I think it would be valuable to identify some entry points from the DevToolsShim that would be nice to log too (thinking about the context menu's inspect element entry mostly). 

This means we need a small refactor of the DevToolsShim in order to reuse the DevToolsStartup's init code. What do you think?

::: toolkit/components/telemetry/Histograms.json:9317
(Diff revision 2)
> +  "DEVTOOLS_ENTRY_POINT": {
> +    "record_in_processes": ["main"],
> +    "bug_numbers": [1378863],
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",

What is the reason behind using enumerated vs categorical (https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#categorical) ?

::: toolkit/components/telemetry/Histograms.json:9320
(Diff revision 2)
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 5,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Records how the user is trigerring Developer Tools installation. (0: Unknown, 1: Key shortcut, 2: System menu, 3: Hamburger menu, 4: Command line argument)"

nit: trigerring -> triggering
Attachment #8884011 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Comment on attachment 8884011 [details]
> Bug 1378863 - Add telemetry probe to track DevTools entry points.
> 
> https://reviewboard.mozilla.org/r/154964/#review165882
> 
> The code seems fine, but according to the documentation
> (https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/
> telemetry/collection/histograms.html#changing-a-histogram) changing an
> existing histogram should not be done lightly. I think it would be valuable
> to identify some entry points from the DevToolsShim that would be nice to
> log too (thinking about the context menu's inspect element entry mostly).

I added ContextMenu as a potential value.
There is some other possibilities, but I'm not sure they are worth tracking:
* scratchpad session restore
* anything that access DevToolsShim.gDevTools, in today's mozilla-central it seems to be only inspectNode and SDK/WebExt methods?
 
> This means we need a small refactor of the DevToolsShim in order to reuse
> the DevToolsStartup's init code. What do you think?

We can also manually call telemetry from there is that's any easier.

> ::: toolkit/components/telemetry/Histograms.json:9317
> (Diff revision 2)
> > +  "DEVTOOLS_ENTRY_POINT": {
> > +    "record_in_processes": ["main"],
> > +    "bug_numbers": [1378863],
> > +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> > +    "expires_in_version": "60",
> > +    "kind": "enumerated",
> 
> What is the reason behind using enumerated vs categorical
> (https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/
> telemetry/collection/histograms.html#categorical) ?

I wasn't aware of the existence of categorical, I based my patch on an existing probe which was using enumerated...
I switched to categorical.
Comment on attachment 8884011 [details]
Bug 1378863 - Add telemetry probe to track DevTools entry points.  datareview=francois

Francois, Could you review this new telemetry probe?
This one would help knowing how web developers toggle Developer Tools.
The precise user actions we are tracking are:
* any DevTools key shortcut like Ctrl/Cmd+Shift+I, Ctrl/Cmd+Shift+C, ...
* "Inspect Element" context menu
* any DevTools command line being passed like --jsconsole
* System menu: Tools > Web Developer
* Hamburger menu: Web Developer
* Customizable toolbar items: the wrench icon
Attachment #8884011 - Flags: feedback?(francois)
Comment on attachment 8884011 [details]
Bug 1378863 - Add telemetry probe to track DevTools entry points.  datareview=francois

https://reviewboard.mozilla.org/r/154964/#review166998

Assuming that the question you want to answer is "what is the relative usage of these various install triggers", then it looks good.

This is Category 2 data (https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories). datareview+

::: toolkit/components/telemetry/Histograms.json:9332
(Diff revision 4)
>      "description": "Type of call: (Bitmask) Audio = 1, Video = 2, DataChannels = 4"
>    },
> +  "DEVTOOLS_ENTRY_POINT": {
> +    "record_in_processes": ["main"],
> +    "bug_numbers": [1378863],
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],

We are now asking for all probes to include the email address of a person responsible for it.

You can keep the team list in there, but please add your own email address (or whoever asked for this probe to be added if you prefer) to this array.
Attachment #8884011 - Flags: review+
Attachment #8884011 - Flags: feedback?(francois)
Comment on attachment 8884011 [details]
Bug 1378863 - Add telemetry probe to track DevTools entry points.  datareview=francois

https://reviewboard.mozilla.org/r/154964/#review168292

Check my comment about guarding the telemetry call to check if the toolbox is already opened. Can be addressed in a follow up if you want.

::: devtools/client/devtools-startup.js:361
(Diff revision 4)
>     */
>    initialized: false,
>  
> -  initDevTools: function () {
> +  initDevTools: function (reason) {
> +    try {
> +      Services.telemetry.getHistogramById("DEVTOOLS_ENTRY_POINT")

Quickly testing this, I feel like the data will be hard to analyze. I think we should only update telemetry if devtools are closed when initDevTools() is called.

Example: I mainly use the keyboard to toggle devtools and switch tools. The counter will increase when I open the tools, everytime I switch to another panel, and when I close the tools.

In comparison, a mouse user will most likely open the tools from the menu, and then click on the tabs to switch tools and finally click on the X icon to close devtools. 

For a similar scenario, my flow would log multiple hits on telemetry, while a mouse user will only log it once. We can keep as is, but we should be very careful when comparing counts.

We have hasToolboxOpened(win) on devtoolsBrowser that we could call before updating telemetry?

::: devtools/client/devtools-startup.js:363
(Diff revision 4)
>  
> -  initDevTools: function () {
> +  initDevTools: function (reason) {
> +    try {
> +      Services.telemetry.getHistogramById("DEVTOOLS_ENTRY_POINT")
> +                        .add(reason);
> +    } catch(e) {

nit: eslint, space after catch
Attachment #8884011 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Comment on attachment 8884011 [details]
> Bug 1378863 - Add telemetry probe to track DevTools entry points.
> 
> https://reviewboard.mozilla.org/r/154964/#review168292
> 
> Check my comment about guarding the telemetry call to check if the toolbox
> is already opened. Can be addressed in a follow up if you want.
> 
> ::: devtools/client/devtools-startup.js:361
> (Diff revision 4)
> >     */
> >    initialized: false,
> >  
> > -  initDevTools: function () {
> > +  initDevTools: function (reason) {
> > +    try {
> > +      Services.telemetry.getHistogramById("DEVTOOLS_ENTRY_POINT")
> 
> Quickly testing this, I feel like the data will be hard to analyze. I think
> we should only update telemetry if devtools are closed when initDevTools()
> is called.

As discussed during the meeting, I updated the patch to only bump the telemetry value if this.initialized is false,
so that we only count the very first action that forces the load of devtools. We end up ignoring all next actions.

I also added my email to the telemetry json file.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f9161013144
Add telemetry probe to track DevTools entry points. r=francois,jdescottes datareview=francois
https://hg.mozilla.org/mozilla-central/rev/3f9161013144
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1428796
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.