Last Comment Bug 695829 - Add telemetry probes to measure how long it takes for us to switch in and out of private browsing mode
: Add telemetry probes to measure how long it takes for us to switch in and out...
Status: RESOLVED FIXED
[Snappy:p2]
:
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 15
Assigned To: :Paolo Amadini
:
Mentors:
: 723075 (view as bug list)
Depends on:
Blocks: 671038
  Show dependency treegraph
 
Reported: 2011-10-19 13:16 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2013-12-27 14:21 PST (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
telemetry (1.71 KB, patch)
2011-10-20 10:36 PDT, (dormant account)
no flags Details | Diff | Review
telemetry for private mode switch (2.32 KB, patch)
2011-10-20 12:36 PDT, (dormant account)
ehsan: review-
Details | Diff | Review
The patch (11.72 KB, patch)
2012-04-10 01:05 PDT, :Paolo Amadini
no flags Details | Diff | Review
Add forgotten test using the wrapper object (12.83 KB, patch)
2012-04-23 13:20 PDT, :Paolo Amadini
no flags Details | Diff | Review
Fix comment (13.21 KB, patch)
2012-04-23 13:24 PDT, :Paolo Amadini
ehsan: review+
Details | Diff | Review
Fix check for data presence in histogram (13.44 KB, patch)
2012-04-25 06:36 PDT, :Paolo Amadini
ehsan: feedback+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2011-10-19 13:16:22 PDT
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) 2011-10-20 10:36:15 PDT
Created attachment 568437 [details] [diff] [review]
telemetry
Comment 2 (dormant account) 2011-10-20 12:36:39 PDT
Created attachment 568477 [details] [diff] [review]
telemetry for private mode switch
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-20 12:45:01 PDT
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] 2011-10-26 12:05:18 PDT
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 5 :Ehsan Akhgari (busy, don't ask for review please) 2012-02-02 12:06:25 PST
*** Bug 723075 has been marked as a duplicate of this bug. ***
Comment 6 Josh Matthews [:jdm] 2012-02-03 05:26:43 PST
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] 2012-02-03 06:43:32 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-02-08 06:54:50 PST
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) 2012-02-26 15:50:04 PST
Ehsan, any update on this?
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-09 14:10:35 PDT
(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 2012-04-10 01:05:37 PDT
Created attachment 613512 [details] [diff] [review]
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-12 11:43:51 PDT
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 2012-04-23 13:20:25 PDT
Created attachment 617616 [details] [diff] [review]
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 2012-04-23 13:24:50 PDT
Created attachment 617621 [details] [diff] [review]
Fix comment

Forgot to update a comment line describing the new test file, sorry.
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-23 15:13:22 PDT
Posted to the try server: http://tbpl.mozilla.org/?tree=Try&rev=f83f5c396ec9
Comment 16 Mozilla RelEng Bot 2012-04-24 02:00:25 PDT
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/eakhgari@mozilla.com-f83f5c396ec9
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 16:03:28 PDT
This is failing tests, Paolo can you please look into that?  Thanks!
Comment 18 :Paolo Amadini 2012-04-24 16:07:30 PDT
(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 2012-04-25 06:36:55 PDT
Created attachment 618233 [details] [diff] [review]
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 19:58:45 PDT
Do you want me to review this patch?
Comment 21 :Paolo Amadini 2012-04-26 07:28:39 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-26 09:11:18 PDT
Please mark this as checkin-needed when you test this on the try server.  Thanks!
Comment 23 :Paolo Amadini 2012-04-27 06:11:44 PDT
Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e532aa7c1d
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-04-29 14:04:40 PDT
http://hg.mozilla.org/mozilla-central/rev/51e532aa7c1d

Note You need to log in before you can comment on or make changes to this bug.