Closed Bug 1232050 Opened 4 years ago Closed 4 years ago

Collect telemetry for number of Sync devices connected.

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 3 obsolete files)

Chris More has been using FHR data to track how the number of devices attached to a sync account affects user retention. We need to start tracking this data using telemetry so the same analysis can be done going forward.

I'm attaching a patch that adds to a keyed histogram. The key is either "desktop" or "mobile" and the number is the number of devices of that type associated with the sync account. Like the FHR data, this count includes the "current" device.

Chris, can you please verify that this probe sounds like it will fit your requirements for retention analysis? In particular, I'd like to make sure that the breakdown by device type is suitable and allows the aggregation you need.

Benjamin, if I'm reading the docs correctly, an "enumerated" probe is the correct choice here ("If you need a linear histogram with buckets < 0, 1, 2 ... N >, then you should declare an enumerated histogram.") n_values=10 which sounds fine for these requirements.
Attachment #8697705 - Flags: review?(benjamin)
Flags: needinfo?(chrismore.bugzilla)
This looks right. I first want to see if they have sync enabled and if they do, how many desktop and mobile devices are connected to the account from Telemetry.

In FHR v2, it was denoted at:

org.mozilla.sync.devices": {
          "_v": 1,
          "desktop": 1,
          "mobile": 1
        },
Flags: needinfo?(chrismore.bugzilla)
I want to ensure we get this on the train to get it lined up for Firefox 44 (1/26/2016) since :trink was building a new cohort rollup based off of v4 in bug 1226379.
Comment on attachment 8697705 [details] [diff] [review]
0001-Bug-XXXXXXX-add-telemetry-for-number-of-devices-atta.patch

In terms of data processing, it's much more efficient to just have two separate histograms WEAVE_DEVICE_COUNT_DESKTOP and WEAVE_DEVICE_COUNT_MOBILE. So I'd recommend that unless there's some reason you want the separate key, you do that.

I think it's important to document that this histogram is recorded each time the client syncs in the description field. Also please document whether the count includes or excludes the "current" device. So e.g. if you only use sync with one device, will this record 0 or 1? Is this recorded only on successful syncs, or all syncs?

Is there a concise statement of the user value we're providing by collecting this data? From our direct conversation ISTR the following things mentioned:

* The new-user churn analysis cohorts by sync usage so that we can measure whether sync is providing user value. If so, we should have a bug to make the churn analysis public.

* Were you going to use this as a denominator for error counts so that we could chart the number of syncs that were failing? If so is that WEAVE_ENGINE_APPLY_NEW_FAILURES? Or some other error metric? 

* If there are other user benefits for this data collection, please document them and who is responsible for analyzing/maintaining the reports.

Please add "bug_numbers" to this histogram to reference this bug.
Attachment #8697705 - Flags: review?(benjamin) → review-
setting P1 and we want to uplift into Fx44
Priority: -- → P1
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> In terms of data processing, it's much more efficient to just have two
> separate histograms WEAVE_DEVICE_COUNT_DESKTOP and
> WEAVE_DEVICE_COUNT_MOBILE. So I'd recommend that unless there's some reason
> you want the separate key, you do that.

You mentioned that you were helping Chris with his retention analysis using telemetry instead of FHR, so I've made this change under the assumption you know that this will work for his use-case.

> I think it's important to document that this histogram is recorded each time
> the client syncs in the description field. Also please document whether the
> count includes or excludes the "current" device. So e.g. if you only use
> sync with one device, will this record 0 or 1? Is this recorded only on
> successful syncs, or all syncs?

Done.

> Is there a concise statement of the user value we're providing by collecting
> this data? From our direct conversation ISTR the following things mentioned:
> 
> * The new-user churn analysis cohorts by sync usage so that we can measure
> whether sync is providing user value. If so, we should have a bug to make
> the churn analysis public.

Chris is updating his existing analysis, so I assume this comment is for him?

> * Were you going to use this as a denominator for error counts so that we
> could chart the number of syncs that were failing? If so is that
> WEAVE_ENGINE_APPLY_NEW_FAILURES? Or some other error metric? 

This is unrelated to other Sync telemetry probes.

> * If there are other user benefits for this data collection, please document
> them and who is responsible for analyzing/maintaining the reports.

That's Chris, and as mentioned above, I believe you said you were assisting him with this. I've no insight into this, but was just asked to make the data available. So again, I assume this is a request for Chris?

> Please add "bug_numbers" to this histogram to reference this bug.

Done.
Assignee: nobody → markh
Attachment #8697705 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8698821 - Flags: review?(benjamin)
Comment on attachment 8698821 [details] [diff] [review]
0001-Bug-1232050-add-telemetry-for-number-of-devices-atta.patch

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

Can we add test coverage for this to test_syncengine_sync.js or a similar test?

::: services/sync/modules/engines/clients.js
@@ +148,5 @@
>      }
>      SyncEngine.prototype._syncStartup.call(this);
>    },
>  
> +  _syncFinish() {

|_syncFinish: function() { ...| ?
(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #6)
> > +  _syncFinish() {
> 
> |_syncFinish: function() { ...| ?

FWIW, the front-end is being fairly aggressive with changing to the more modern object definitions when touching them, so unless there is a strong objection I'd prefer to keep doing that here.
Should the WEAVE_CONFIGURED histogram definition be updated to include "releaseChannelCollection": "opt-out" too?
(In reply to Mark Reid [:mreid] from comment #8)
> Should the WEAVE_CONFIGURED histogram definition be updated to include
> "releaseChannelCollection": "opt-out" too?

Yes, I think it should, thanks. This patch includes that.
Attachment #8698821 - Attachment is obsolete: true
Attachment #8698821 - Flags: review?(benjamin)
Attachment #8704886 - Flags: review?(benjamin)
Comment on attachment 8704886 [details] [diff] [review]
0001-Bug-1232050-add-telemetry-for-number-of-devices-atta.patch

From a separate email thread:
* sync team is responsible for the correctness of this data
* At the present time, the churn report correlating churn against new user retention is the only artifact driven by this data, but the team has a plan to add other quality metrics and reporting over the next quarter. Chris More and Chris Karlof are both responsible for monitoring the churn report.
* The immediate user benefit is measuring and optimizing the sync experience and activation flow, and that is going to drive product improvements for the next year at least.

Please document that WEAVE_CONFIGURED is only recorded once per session (near startup). This is not the ideal configuration since this metric will not appear in any but the first subsession; please file a followup to record this either in the environment or some more persistent flag space.

data-steward approval=me
Attachment #8704886 - Flags: review?(benjamin) → feedback+
Thanks Benjamin.

This patch adds the requested comment re FXA_CONFIGURED. Kit, are you able to review this fairly soon?
Attachment #8704886 - Attachment is obsolete: true
Attachment #8706187 - Flags: review?(kcambridge)
Thanks Benjamin!
Comment on attachment 8706187 [details] [diff] [review]
0001-Bug-1232050-add-telemetry-for-number-of-devices-atta.patch

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

Looks good!
Attachment #8706187 - Flags: review?(kcambridge) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Please document that WEAVE_CONFIGURED is only recorded once per session
> (near startup). This is not the ideal configuration since this metric will
> not appear in any but the first subsession; please file a followup to record
> this either in the environment or some more persistent flag space.

I opened bug 1238810
> 
> data-steward approval=me
Comment on attachment 8706187 [details] [diff] [review]
0001-Bug-1232050-add-telemetry-for-number-of-devices-atta.patch

Approval Request Comment
[Feature/regressing bug #]: Unified telemetry
[User impact if declined]: We are unable to measure Sync retention stats and how Sync changes general Firefox retention metrics
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: Low risk, limited to Sync
[String/UUID change made/needed]: None
Attachment #8706187 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/81427927d338
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment on attachment 8706187 [details] [diff] [review]
0001-Bug-1232050-add-telemetry-for-number-of-devices-atta.patch

Sure, taking it in 45.
Attachment #8706187 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.