1249295 - Telemetry to track popular device entries

Status: NEW

(DevTools :: Responsive Design Mode, defect, P3)

Description

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

Along with adding a device list, we should also track with telemetry which device options are most popular, to aid in future tuning of the list.

Comment 1

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

:clarkbw, what data would we like to have here?  Some ideas:

1. Each time a device is selected for a viewport, increment a telemetry probe keyed by the device ID
2. In the "active devices" modal, record user changes (added or removed) that differ from the default device set keyed by device ID

Do you want both of these?  Is there anything else?  Should they be opt-out for release?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> :clarkbw, what data would we like to have here?  Some ideas:
> 
> 1. Each time a device is selected for a viewport, increment a telemetry
> probe keyed by the device ID

This is what I was thinking, devices that aren't used could probably be dropped from the list in favor of others.  

And currently we default to nothing which would really help for these numbers to be more accurate than the opened count for things like the inspector which are likely the panel opened by default.

> 2. In the "active devices" modal, record user changes (added or removed)
> that differ from the default device set keyed by device ID

I hadn't considered this and I like it.  If we could get the device sizes added we could answer the question of what devices we are missing.  We would have to reverse lookup those devices by their size but it wouldn't be that hard to do.

> Do you want both of these?  

Probably yes. Both would answer the questions What is working? and What are we missing?

> Is there anything else?

I don't think so.

> Should they be opt-out for release?

Yes for 1. I think 2 might be a no.

Comment 3

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

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #2)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> > :clarkbw, what data would we like to have here?  Some ideas:
> > 
> > 1. Each time a device is selected for a viewport, increment a telemetry
> > probe keyed by the device ID
> 
> This is what I was thinking, devices that aren't used could probably be
> dropped from the list in favor of others.  
> 
> And currently we default to nothing which would really help for these
> numbers to be more accurate than the opened count for things like the
> inspector which are likely the panel opened by default.

Do we want to record "no device selection" somehow?  I am thinking about how to answer the question "Is the device selector frequently used during an RDM session?" to validate the utility of the device selector.  We can sum all the times a device is chosen to compute the numerator, but what's the right denominator?

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Sure, perhaps one of the fields is "resizer" so we know that a non device was selected?  If we could avoid the default value polluting the selection it would help with the metrics.

Comment 5

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

Attachment: Bug 1249295 - Rename viewport.device to deviceName.

Review commit: https://reviewboard.mozilla.org/r/76198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76198/

Comment 6

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

Attachment: Bug 1249295 - Record device selection and resizer usage in RDM. p=bsmedberg

Review commit: https://reviewboard.mozilla.org/r/76200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76200/

Comment 7

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

Attachment: Bug 1249295 - Record user's preferred devices in RDM. p=bsmedberg

Review commit: https://reviewboard.mozilla.org/r/76202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76202/

Comment 8

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

Comment on attachment 8787449 [details]
Bug 1249295 - Record device selection and resizer usage in RDM.  p=bsmedberg

:bsmedberg, this adds a new probe to track usage of a device selector in Responsive Design Mode.  The probe is opt-out for release to ensure we get good data from our main user base.  :clarkbw will include this in the DevTools dashboards.

This probe is keyed by device ID because there's a large set of devices, and we intend to update the device list over time.  In the future, we also plan to allow user-defined devices.

Comment 9

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

Comment on attachment 8787450 [details]
Bug 1249295 - Record user's preferred devices in RDM.  p=bsmedberg

:bsmedberg, this adds 2 new probes to track "preferred devices" that users have chosen to add to or remove from the device selector (they are chosen from a modal with a large database of possible devices).  The probe is opt-in for release (the default).

This probe is keyed by device ID because there's a large set of devices, and we intend to update the device list over time.  In the future, we also plan to allow user-defined devices.  It's enumerated to track added vs. removed devices.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8787448 [details]
Bug 1249295 - Rename viewport.device to deviceName.

https://reviewboard.mozilla.org/r/76198/#review74794

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8787449 [details]
Bug 1249295 - Record device selection and resizer usage in RDM.  p=bsmedberg

https://reviewboard.mozilla.org/r/76200/#review74774

Looks good to me.

::: devtools/client/responsive.html/actions/viewports.js:31
(Diff revision 1)
>  
>    /**
>     * Change the viewport device.
>     */
>    changeDevice(id, deviceName) {
> +    if (deviceName != "") {

nit: if (deviceName) ?

::: devtools/client/responsive.html/components/resizable-viewport.js:77
(Diff revision 1)
>      });
> +
> +    // Moving this to a "stop resizing" action creator seems more in the spirit of Redux.
> +    // Since the resizing boolean is currently not exposed to other components, making
> +    // such an action creator and state just for telemetry seems a bit overboard. Anyway,
> +    // if we later add such a thing, move this to the action creator.

That may be me who know nothing about flux, redux & cie. But isn't this comment bit extensive?
Seeing logResizerUsed() being called in onResizeStop looks totally fine to me.

::: devtools/client/responsive.html/test/browser/browser_device_change.js:17
(Diff revision 1)
>  const NOKIA_UA = "Mozilla/5.0 (compatible; MSIE 10.0; Windows Phone 8.0; " +
>    "Trident/6.0; IEMobile/10.0; ARM; Touch; NOKIA; Lumia 520)";
> -const Types = require("devtools/client/responsive.html/types");
>  
> +const RDM_OPENED_HISTOGRAM = "DEVTOOLS_RESPONSIVE_OPENED_COUNT";
> +const DEVICE_SELECTED_HISTOGRAM = "DEVTOOLS_RESPONSIVE_DEVICE_SELECTED";

It may be handy to pull that from telemetry.js?

::: devtools/client/responsive.html/test/browser/browser_device_change.js:92
(Diff revision 1)
> +  testTelemetryResult(results, 3, `${DEVICE_SELECTED_HISTOGRAM}|Laptop (1366 x 768)`);
> +}
> +
> +function testTelemetryResult(results, index, histId) {
> +  is(results[index][0], histId, `Logged result ${index} for ${histId}`);
> +  ok(results[index][1], `Logged result ${index} with value true`);

Given that this is kind: "count", shouldn't we assert some incremental number somewhere? Right here you only assert a boolean. I would have expect to see `is(something, 1)`.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8787450 [details]
Bug 1249295 - Record user's preferred devices in RDM.  p=bsmedberg

https://reviewboard.mozilla.org/r/76202/#review74790

Are you sure this probe is useful?
We can easily see which device is useless by seing the one that are never actually used/selected.
I imagine we can easily live with just the previous patch.

::: devtools/client/responsive.html/actions/devices.js:105
(Diff revision 1)
>    loadDevices() {
>      return function* (dispatch, getState) {
>        yield dispatch({ type: LOAD_DEVICE_LIST_START });
>        let preferredDevices = loadPreferredDevices();
> +      // Log existing preferred devices during RDM open
> +      telemetry.logPreferredDevices(preferredDevices);

I don't get why we probe that in telemetry?
Isn't the other one enough? Only when something change...
At first, it looks like this probe will just duplicate the other one.

::: devtools/client/responsive.html/test/browser/browser_device_modal_submit.js:26
(Diff revision 1)
> -const Types = require("devtools/client/responsive.html/types");
>  
> +const RDM_OPENED_HISTOGRAM = "DEVTOOLS_RESPONSIVE_OPENED_COUNT";
> +const PREFERRED_DEVICES_HISTOGRAM = "DEVTOOLS_RESPONSIVE_PREFERRED_DEVICES";
> +const PREFERRED_DEVICES_CHANGED_HISTOGRAM =
> +  "DEVTOOLS_RESPONSIVE_PREFERRED_DEVICES_CHANGED";

Same than previous patch, it could be handy to pull that from telemetry.js

::: devtools/client/responsive.html/test/browser/browser_device_modal_submit.js:171
(Diff revision 1)
> +  let results = Object.entries(Telemetry.prototype.telemetryInfo);
> +  for (let result of results) {
> +    info(`T: ${result[0]} ${result[1]}`);
> +  }
> +  testTelemetryResult(results, 0, RDM_OPENED_HISTOGRAM, [ true, true ]);
> +  // Device modal is submitted twice while with the following device added

s/while// ?

::: devtools/client/responsive.html/test/browser/browser_device_modal_submit.js:174
(Diff revision 1)
> +  }
> +  testTelemetryResult(results, 0, RDM_OPENED_HISTOGRAM, [ true, true ]);
> +  // Device modal is submitted twice while with the following device added
> +  testTelemetryResult(results, 1,
> +    `${PREFERRED_DEVICES_CHANGED_HISTOGRAM}|${firstUncheckedDevice}`, [ 1, 1 ]);
> +  // Device modal is submitted once while with the following device removed

s/while// ?

::: devtools/client/responsive.html/test/browser/browser_device_modal_submit.js:181
(Diff revision 1)
> +    `${PREFERRED_DEVICES_CHANGED_HISTOGRAM}|${firstCheckedDevice}`, [ 0 ]);
> +  // The above device modifications are reported during the second RDM open in this test
> +  testTelemetryResult(results, 4,
> +    `${PREFERRED_DEVICES_HISTOGRAM}|${firstCheckedDevice}`, [ 0 ]);
> +  testTelemetryResult(results, 5,
> +    `${PREFERRED_DEVICES_HISTOGRAM}|${firstUncheckedDevice}`, [ 1 ]);

Wouldn't it be clearer to name these firstSomething variables with `g` prefix or a capital case to clearly state this is from global scope?

Comment 13

[Benjamin Smedberg]

Comment on attachment 8787449 [details]
Bug 1249295 - Record device selection and resizer usage in RDM.  p=bsmedberg

https://reviewboard.mozilla.org/r/76200/#review75140

::: toolkit/components/telemetry/Histograms.json:7397
(Diff revision 1)
> +    "bug_numbers": [1249295],
> +    "expires_in_version": "58",
> +    "kind": "count",
> +    "keyed": true,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Increments each time a device is selected in Responsive Design Mode keyed by the device ID.  As a special case, the ID '<Resizer>' records changing viewport size with the grippers."

I am concerned with your statement "This probe is keyed by device ID because there's a large set of devices, and we intend to update the device list over time.  In the future, we also plan to allow user-defined devices."

I don't think it's a problem to record predefined device IDs, although I'd like this description to link to where we keep those list of IDs.

I have concerns about recording user-customized data. What will the IDs look like in this case? Will it include the actual "name" of the device that the user entered? This seems like an identification/privacy risk.

Comment 14

[Benjamin Smedberg]

Comment on attachment 8787450 [details]
Bug 1249295 - Record user's preferred devices in RDM.  p=bsmedberg

https://reviewboard.mozilla.org/r/76202/#review75146

::: toolkit/components/telemetry/Histograms.json:7406
(Diff revision 1)
> +    "bug_numbers": [1249295],
> +    "expires_in_version": "58",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "keyed": true,
> +    "description": "Records changes by the user (added or removed) to preferred devices in Responsive Design Mode keyed by the device ID.  Values are recorded on each opening of Responsive Design Mode. (0: Removed, 1: Added)"

Why is n_values=10 if you're only ever recording 0/1? I know we typically reserve a few values for expansion of actual enumerations, but this seems like a boolean which we are unlikely to expand.

Also, same question as the other patch about linking to the list of device IDs and dealing with custom device IDs.

Do you really need to record this on every opening of RDM? Is it possible that users will do this lots of times per session? I wonder if a scalar string/list of strings that is recorded once per session would be easier to analyze.

Comment 15

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

(In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> Comment on attachment 8787449 [details]
> Bug 1249295 - Record device selection and resizer usage in RDM.  p=bsmedberg
> 
> https://reviewboard.mozilla.org/r/76200/#review75140
> 
> ::: toolkit/components/telemetry/Histograms.json:7397
> (Diff revision 1)
> > +    "bug_numbers": [1249295],
> > +    "expires_in_version": "58",
> > +    "kind": "count",
> > +    "keyed": true,
> > +    "releaseChannelCollection": "opt-out",
> > +    "description": "Increments each time a device is selected in Responsive Design Mode keyed by the device ID.  As a special case, the ID '<Resizer>' records changing viewport size with the grippers."
> 
> I am concerned with your statement "This probe is keyed by device ID because
> there's a large set of devices, and we intend to update the device list over
> time.  In the future, we also plan to allow user-defined devices."
> 
> I don't think it's a problem to record predefined device IDs, although I'd
> like this description to link to where we keep those list of IDs.

The device list with IDs and other data is available at https://github.com/mozilla/simulated-devices.  I can update the probe description to include this.

> I have concerns about recording user-customized data. What will the IDs look
> like in this case? Will it include the actual "name" of the device that the
> user entered? This seems like an identification/privacy risk.

The user-defined devices feature doesn't exist yet, but we would need to normalize such devices before submitting to telemetry to get any use out of the data (since each user could make any name they like for the same device).  So, I would imagine we would generate some ID for user devices which just includes the size parameters instead of the name.

Comment 16

[Benjamin Smedberg]

Comment on attachment 8787449 [details]
Bug 1249295 - Record device selection and resizer usage in RDM.  p=bsmedberg

https://reviewboard.mozilla.org/r/76200/#review75170

ok data-review=me for this part if you add the external link to the device list

Comment 17

[Benjamin Smedberg]

Comment on attachment 8787450 [details]
Bug 1249295 - Record user's preferred devices in RDM.  p=bsmedberg

I'm still concerned about the performance characteristics/behavior of this part.

Comment 18

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

Clearing ni? for now as more important bugs have come up.  Will reply with more detail when this bug is revisited.