723075 - batch 'o telemetry metrics for private browsing transitions

Status: RESOLVED DUPLICATE of bug 695829

(Firefox :: Private Browsing, defect)

Description

[:Paolo Amadini]

Add telemetry metrics for private browsing transitions (meta-bug: 671038).

Time interval measurements that can make sense:
- From before "private-browsing-change-granted" observers are called
    to after "private-browsing" observers are called
- From before "private-browsing-change-granted" observers are called
    to after "private-browsing-transition-complete" observers are called

Distinguished by these conditions:
- Entering PBM because of explicit user action
- Exiting PBM because of explicit user action
- Entering PBM automatically on startup

Comment 1

[:Paolo Amadini]

Attachment: The patch

This adds telemetry measurements for the time it takes to enter and exit
private browsing mode. The actual timing data is reported to the telemetry
service when PBM ends, since data collection is disabled during PBM.

These measurements include all observer notifications, except those that are
expected to create a confirmation dialog and wait for input. This should help
in determining if users are observing poor performance in some cases.

Note that, if PBM autostart is enabled or the browser is closed before exiting
PBM, then the data is never reported.

Comment 2

[(no longer active)]

This is a dupe of bug 695829, which has a contributor working on it.  Since you've written a patch, I'm going to review it here, but please coordinate with Allen on this work.  Thanks!

Comment 3

[(no longer active)]

Comment on attachment 593836 [details] [diff] [review]
The patch

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

::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
@@ +321,5 @@
> +      this._enterTimestamps[aPhase] = Date.now();
> +    } else {
> +      if (this._quitting) {
> +        // If we are quitting the browser, we don't care collecting the data,
> +        // because we wouldn't be able to record it with telemetry.

I thought that the telemetry service has been recently modified to enable reporting of such data?

@@ +348,5 @@
> +         "PRIVATE_BROWSING_TRANSITION_EXIT_TOTAL_MS",
> +         this._exitTimestamps.completed - this._exitTimestamps.started);
> +  },
> +
> +  _reportTelemetryEntry: function PBS__reportTelemetryEntry(aHistogramId,

Please make this a local function inside _reportTelemetry.

::: browser/components/privatebrowsing/test/unit/test_privatebrowsing_telemetry.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gPrivateBrowsing",
> +                                   "@mozilla.org/privatebrowsing;1",
> +                                   "nsIPrivateBrowsingService");

For the PB service tests, we test things both using the internal service and the wrapper service.

In order to do this, you need to do the following:

1. Move this file to be called do_test_privatebrowsing_telemetry.js.  Rename the run_test function to do_test, and use PRIVATEBROWSING_CONTRACT_ID instead of the hard-coded contract ID.
2. Create a test_privatebrowsing_telemetry.js file similar to http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/unit/test_privatebrowsing_autostart.js for testing the internal service.
3. Create a test_privatebrowsingwrapper_telemetry.js file similar to http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/unit/test_privatebrowsingwrapper_autostart.js for testing the wrapper service.

Comment 4

[Dietrich Ayala (:dietrich)]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> This is a dupe of bug 695829, which has a contributor working on it.  Since
> you've written a patch, I'm going to review it here, but please coordinate
> with Allen on this work.  Thanks!
> 
> *** This bug has been marked as a duplicate of bug 695829 ***

There's no Allen anywhere on the other bug afaict, only a patch from Taras from last October...

Should Paolo move his patch over there, and take over?

Comment 5

[:Paolo Amadini]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> I thought that the telemetry service has been recently modified to enable
> reporting of such data?

We still have to wait until we are out of Private Browsing Mode. I just talked with Taras,
and will move the patch over to the other bug. I'll be able to get at it next week probably.
Thank you Ehsan for the clear review!