695829 - Add telemetry probes to measure how long it takes for us to switch in and out of private browsing mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(no longer active)]

This method is responsible for toggling the state:

http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js?force=1#497

Specifically, the probe needs to measure how long it takes for us from line 522 (setting the status to STATE_TRANSITION_STARTED) to the end of the method.  The pieces before line 522 can potentially show UI, so we shouldn't measure those parts (since we'd be measuring user's response time ;-).

We should ideally have two probes, one for entering and one for leaving the private browsing mode.

Comment 1

[(dormant account)]

Attachment: telemetry

Comment 2

[(dormant account)]

Attachment: telemetry for private mode switch

Comment 3

[(no longer active)]

Comment on attachment 568477 [details] [diff] [review]
telemetry for private mode switch

r- because most of the interesting stuff in private browsing mode switching happens during the "private-browsing" notification.  I'm going to write a patch on top of this one which is going to turn off recording in the telemetry service right before we record this histogram, and turn it back on if it was enabled to begin with.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

While measuring how long it takes to enter/exit private browsing mode also implies usage of the feature, that too is in scope for the updated opt-in string caused by fixing bug 685373.  There's no privacy risk analysis needed here, so clearing the review flag.

Comment 6

[Josh Matthews [:jdm]]

I'm assigning to ehsan for now to avoid other people snapping it up while we figure out the status of the work being done by an academic group.

Comment 7

[Marco Bonardo [:mak]]

It's unclear why that effort has not been documented elsewhere, especially in the bug. Would have saved our developers time in trying to fix an apparently unowned bug.

Comment 8

[(no longer active)]

Sorry for the confusion here, guys.  Allen has been working on this bug and we've been talking about this over email, and he has a work in progress patch.  Paolo's patch here <https://bugzilla.mozilla.org/attachment.cgi?id=593836>  is more complete though, and I'd like to move forward with that.  I'll talk to Allen to see if he's willing to continue based on Paolo's patch or it he's willing to work on something else instead, and will update the bug accordingly.

Comment 9

[Dietrich Ayala (:dietrich)]

Ehsan, any update on this?

Comment 10

[(no longer active)]

(In reply to Dietrich Ayala (:dietrich) from comment #9)
> Ehsan, any update on this?

No, I have not heard back from either Allen or Paolo on this, I don't think.

Comment 11

[:Paolo Amadini]

Attachment: The patch

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to Dietrich Ayala (:dietrich) from comment #9)
> > Ehsan, any update on this?
> 
> No, I have not heard back from either Allen or Paolo on this, I don't think.

So, I think we can continue from where we left on bug 723075.

Comment 12

[(no longer active)]

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

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

This looks mostly good except for the comments below.  I'd like to look at another version of the patch which fixes these comments.

::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
@@ +338,5 @@
> +    } 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.
> +        return;

So, can the telemetry service submit the data gathered in the last run when Firefox gets restarted now?

::: browser/components/privatebrowsing/test/unit/test_privatebrowsing_autostart.js
@@ +39,4 @@
>  
>  function run_test() {
>    PRIVATEBROWSING_CONTRACT_ID = "@mozilla.org/privatebrowsing;1";
> +  load("do_test_privatebrowsing_telemetry.js");

Please also add another test named test_privatebrowsingwrapper_telemetry.js which tests using the -wrapper PRIVATEBROWSING_CONTRACT_ID, similar to the other "wrapper" tests in this directory.

Comment 13

[:Paolo Amadini]

Attachment: Add forgotten test using the wrapper object

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> So, can the telemetry service submit the data gathered in the last run when
> Firefox gets restarted now?

I couldn't see a similar change, from a quick look at the revision history.
The fact is we weren't able to gather data at that point during shutdown.

Taras might know more?

Comment 14

[:Paolo Amadini]

Attachment: Fix comment

Forgot to update a comment line describing the new test file, sorry.

Comment 15

[(no longer active)]

Posted to the try server: http://tbpl.mozilla.org/?tree=Try&rev=f83f5c396ec9

Comment 16

[Mozilla RelEng Bot]

Try run for f83f5c396ec9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f83f5c396ec9
Results (out of 218 total builds):
    exception: 4
    success: 171
    warnings: 43
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f83f5c396ec9

Comment 17

[(no longer active)]

This is failing tests, Paolo can you please look into that?  Thanks!

Comment 18

[:Paolo Amadini]

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> This is failing tests, Paolo can you please look into that?  Thanks!

Yes, the telemetry test fails intermittently. Another run here:

https://tbpl.mozilla.org/?tree=Try&rev=da6dbfde78f0

I'll look into this.

Comment 19

[:Paolo Amadini]

Attachment: Fix check for data presence in histogram

The issue was probably that we check the "sum" property to verify that we have
data in the histogram, but it is zero when the time it takes for a transition
is not measurable. In fact, the tests failed on Windows, where we have a coarse
timer resolution.

Checking if we have some data in the zeroth bucket, in addition to the sum
calculated for the other buckets, apparently solves the issue.

Comment 20

[(no longer active)]

Do you want me to review this patch?

Comment 21

[:Paolo Amadini]

Comment on attachment 618233 [details] [diff] [review]
Fix check for data presence in histogram

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> Do you want me to review this patch?

Sure, feel free to review it, the only change is how we check the test result.

Comment 22

[(no longer active)]

Please mark this as checkin-needed when you test this on the try server.  Thanks!

Comment 23

[:Paolo Amadini]

Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e532aa7c1d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/51e532aa7c1d