Closed Bug 1013448 Opened 5 years ago Closed 5 years ago

Add various sync telemetry probes, including master password usage in conjunction with Sync

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox30 - ---

People

(Reporter: ckarlof, Assigned: markh)

Details

(Whiteboard: p=3 s=it-32c-31a-30b.3 [qa!])

Attachments

(1 file, 4 obsolete files)

This will help us better estimate how many users have master password enabled and how many users that have master password enabled also use Sync.
Gavin, I'd like to get this uplifted to 30.
Whiteboard: [qa+]
My situation will probably fool you.  My home computer is secure enough (IMHO) that there is no master password.

My travel netbook has one.  But it's in the drawer right now awaiting my next trip, so your telemetry will not count it!

Of course this means I really want to sync between a machine with and one without a master password, so a solution which depends on the same master for both will not help.
I don't think we need to track this, but we can uplift a patch.
(In reply to Marc Auslander from comment #2)
> My situation will probably fool you.  My home computer is secure enough
> (IMHO) that there is no master password.
> 
> My travel netbook has one.  But it's in the drawer right now awaiting my
> next trip, so your telemetry will not count it!

I acknowledge this approach is far from ideal. If you know of better way to estimate these statistics, please let me know. This is the best I'm aware of at this point in time.
Given we'd like to get this into 30, adding to the fx-backlog - Marco, can you please add this to the current iteration?
Assignee: nobody → mhammond
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Whiteboard: [qa+] → p=3 [qa+]
Added to Iteration 32.2
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Whiteboard: p=3 [qa+] → p=3 s=it-32c-31a-30b.2 [qa+]
This is a first cut.  It adds 4 probes (and note I used a "WEAVE_" prefix, as a "SYNC_" prefix might upset people who are looking for sync-vs-async probes)

* A boolean to indicate if sync is configured at all for this device.
* A boolean to indicate if sync is configured *and* the user has MP enabled.  (Note there is no probe to indicate if it is actually locked as a sync starts, but I'm not sure that is necessary)

[I wonder though if we should just have a probe for "is MP enabled" independent of sync - but then I'm not sure how we could correlate this figure with the number of people with *both* sync and MP]

2 additional probes that may help us with the migration planning (specifically, we probably want to make sure there aren't many incomplete syncs):

* The number of times sync was started in the session.
* The number of times sync completed without errors in the session.

General feedback requested, and if anyone knows someone intimate with telemetry who I can also ping, please let me know.
Attachment #8425972 - Flags: feedback?(rnewman)
Attachment #8425972 - Flags: feedback?(ckarlof)
Comment on attachment 8425972 [details] [diff] [review]
0001-Add-telemetry-probes-for-sync.-r.patch

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

Mark, this is a great start. I also want a probe that indicates whether MP is enabled at all, regardless if Sync is enabled or not. I don't think this patch covers that, correct? I understand this might cover some non-Sync code.
Attachment #8425972 - Flags: feedback?(ckarlof) → feedback+
Comment on attachment 8425972 [details] [diff] [review]
0001-Add-telemetry-probes-for-sync.-r.patch

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

Concur with ckarlof: add an entirely separate probe for whether MP is enabled. That lets us estimate MP vs no MP, Sync vs no Sync, and the size of the Sync + MP group.

We also get to estimate what impact MP has on sync frequency: we won't sync if MP is locked.

::: services/sync/Weave.js
@@ +77,5 @@
> +    let histogram = Services.telemetry.getHistogramById("WEAVE_CONFIGURED");
> +    histogram.add(1);
> +    if (Utils.mpEnabled()) {
> +      let histogram = Services.telemetry.getHistogramById("WEAVE_CONFIGURED_MASTERPASSWORD");
> +      histogram.add(1);

Can we add the 0 case, too? Basic sanity check that count(0) + count(1) = count(WEAVE_CONFIGURED).

::: toolkit/components/telemetry/Histograms.json
@@ +5990,5 @@
> +  },
> +  "WEAVE_COMPLETE_SUCCESS_COUNT": {
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": 100,

Be prepared to alter this bucketing; we expect syncs to be triggered on user activity, as well as every few minutes, so having Firefox open for the work day will peg all of those users into the top bucket.
Attachment #8425972 - Flags: feedback?(rnewman) → feedback+
Following up from our conversation on IRC, this probe is done at the end of delayedStartup.
Attachment #8425972 - Attachment is obsolete: true
Attachment #8426740 - Flags: review?(dolske)
Attached patch sync-specific probes (obsolete) — Splinter Review
Similar to the last patch, but I removed the CONFIGURED_WITH_MASTER_PASSWORD probe in favour of the "part 1", and always call add() for the WEAVE_CONFIGURED probe (ie, it is always called with 0 or 1, rather than only called when true)
Attachment #8426743 - Flags: review?(rnewman)
Comment on attachment 8426740 [details] [diff] [review]
Add a probe for master-password usage.

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

Are you sure you don't also want the WEAVE_CONFIGURED_MASTERPASSWORD probe from the earlier patch?

Just knowing the independent states of "user has enabled Sync" and "user has MP" does let you _estimate_ "user has Sync+MP" as the product of the two. But if you specifically want data on the Sync+MP state, it would be worthwhile to specifically probe that. I fear that between selection bias and the small numbers of people with either enabled, that such an estimate might not be as accurate as we'd like.

Also, AIUI, Old-Sync could be used with the MP, but New-Sync can not... Given that the New Sync is more usable and actively promoted, that would seem to further skew the accuracy of an estimate.
Attachment #8426740 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #12)
 
> Are you sure you don't also want the WEAVE_CONFIGURED_MASTERPASSWORD probe
> from the earlier patch?

I believe we do want that.
(In reply to Justin Dolske [:Dolske] from comment #12)

> Just knowing the independent states of "user has enabled Sync" and "user has
> MP" does let you _estimate_ "user has Sync+MP" as the product of the two.
> But if you specifically want data on the Sync+MP state, it would be
> worthwhile to specifically probe that. I fear that between selection bias
> and the small numbers of people with either enabled, that such an estimate
> might not be as accurate as we'd like.

hrmph - I (erroneously) concluded that we should be able to correlate this, as otherwise I'd have found many other "composite" probes.  So color me surprised :)

> Also, AIUI, Old-Sync could be used with the MP, but New-Sync can not...

That's not quite correct - new sync will simply refuse to allow passwords to sync with an MP.

(In reply to Chris Karlof [:ckarlof] from comment #13)
> I believe we do want that.

OK, I'll update part 2 to re-add this probe.

Thanks!
Updated as discussed.  I also neglected to mention that these patches use "exponential" instead of "linear", with a "high_value" of 1000 - hopefully this addresses:

(In reply to Richard Newman [:rnewman] from comment #9)
> Be prepared to alter this bucketing; we expect syncs to be triggered on user
> activity, as well as every few minutes, so having Firefox open for the work
> day will peg all of those users into the top bucket.
Attachment #8426743 - Attachment is obsolete: true
Attachment #8426743 - Flags: review?(rnewman)
Attachment #8427435 - Flags: review?(rnewman)
Comment on attachment 8427435 [details] [diff] [review]
0002-Bug-1013448-part-2-add-telemetry-probes-for-sync.-r-.patch

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

::: services/sync/modules/service.js
@@ +1247,5 @@
>      return this._lock("service.js: sync",
>                        this._notify("sync", "", function onNotify() {
>  
> +      let histogram = Services.telemetry.getHistogramById("WEAVE_START_COUNT");
> +      histogram.add(1);

You'd think there'd be a S.t helper for this, wouldn't you?

::: toolkit/components/telemetry/Histograms.json
@@ +5988,5 @@
> +  },
> +  "WEAVE_CONFIGURED": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "If sync is configured for this device."

"If any version of Firefox Sync is configured for this device."

@@ +5993,5 @@
> +  },
> +  "WEAVE_CONFIGURED_MASTER_PASSWORD": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "If sync is configured with a master-password for this device."

"If both Firefox Sync and Master Password are configured for this device."
Attachment #8427435 - Flags: review?(rnewman) → review+
sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=40386222&tree=Fx-Team
Joel, Roberto, got a clue why the xperf test fails like this?

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\secmod.db' was accessed and we were not expecting it.  DiskReadCount: 6, DiskWriteCount: 0, DiskReadBytes: 16904, DiskWriteBytes: 0

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\cert8.db' was accessed and we were not expecting it.  DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 33288, DiskWriteBytes: 0

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\key3.db' was accessed and we were not expecting it.  DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 8712, DiskWriteBytes: 0
Flags: needinfo?(rvitillo)
Flags: needinfo?(jmaher)
The patch causes these files to be touched as part of normal Fx startup.  IIUC, this would be sync IO, so the tests saved us :)  I'll deal with this tomorrow.
Flags: needinfo?(rvitillo)
Flags: needinfo?(jmaher)
This XPerf test checks for new sources of main-thread I/O taking place before the "sessionstore-windows-restored" event. I think this patch caused the security DBs to be accessed earlier by touching the "pkcs11moduledb" component in _delayedStartup().

I think you can fix this by moving the Telemetry work out of _delayedStartup. Either do it after sessionstore-windows-restored (i.e. listen for this event and then do the work in a setTimeout) or at browser exit ("profile-before-change" event).
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
I'm going to change the scope of this bug to just the Sync probes - they don't have the problem that caused the backout, and may take longer to land as we decide how to mitigate the main-thread sync IO.
Summary: Add telemetry probes for master password usage and master password usage in conjunction with Sync → Add various sync telemetry probes, including master password usage in conjunction with Sync
please ping me if you have trouble sorting this out and getting a green Talos Xperf run.
https://hg.mozilla.org/mozilla-central/rev/a6bf99f60178
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
FTR, bug 1016138 is for the master-password telemetry.
Assigning to Tracy for verification since it's related to Sync. Tracy, let us know if you need any help with this.
QA Contact: twalker
Hi Tracy, will it be possible to have this bug verified by end of day this Friday?  Our current Iteration ends on Monday June 9.
Flags: needinfo?(twalker)
Joel, I don't know much about telemetry probes. What are the steps to verify this is collecting the necessary data?
Flags: needinfo?(jmaher)
oh, I have no clue- my participation in this bug was related to the talos failure.  I assume we could see this on the telemetry dashboard, I see a weave master password:
http://telemetry.mozilla.org/#nightly/32/WEAVE_CONFIGURED_MASTER_PASSWORD

I am not sure if that is it or what the data means, but that specific data point has values being reported at some point in time!
Flags: needinfo?(jmaher)
Joel, ok, that is actually enough for me to figure this out.  I think looking for WEAVE_CONFIGURED_MASTER_PASSWORD in about:telemetry > Histograms should do it.  Thanks.
success! :-)  

With Master Password enabled and logged into Sync, in about:telemetry > Histograms I see entries for:
WEAVE_COMPLETE_SUCCESS_COUNT
WEAVE_CONFIGURED
WEAVE_CONFIGURED_MASTER_PASSWORD
WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION
WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION
WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION
WEAVE_START_COUNT
Status: RESOLVED → VERIFIED
Flags: needinfo?(twalker)
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa!]
You need to log in before you can comment on or make changes to this bug.