Last Comment Bug 723090 - Add telemetry probe for the opening time of the site identity popup
: Add telemetry probe for the opening time of the site identity popup
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
Depends on:
Blocks: 671038 671044
  Show dependency treegraph
 
Reported: 2012-02-01 06:23 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-02-04 02:35 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 723090 (4.43 KB, patch)
2012-02-01 08:14 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug (4.84 KB, patch)
2012-02-02 02:11 PST, Jared Wein [:jaws] (please needinfo? me)
dietrich: review+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-02-01 06:23:51 PST

    
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-02-01 08:14:52 PST
Created attachment 593458 [details] [diff] [review]
Patch for bug 723090

Dietrich: With this patch applied, I get a lot of 0 millisecond duration data points added to the histogram for times when the site identity popup hasn't been opened.

I'm not sure if it is something wrong with my computer or with the patch that I have written. Can you apply the patch and see if you get the same issue?
Comment 2 Dietrich Ayala (:dietrich) 2012-02-01 14:12:12 PST
Comment on attachment 593458 [details] [diff] [review]
Patch for bug 723090

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

I don't see anything that would cause values of 0 to be submitted. That is something you could control at the point of recording, but that shouldn't happen if the popup isn't actually being shown. You could do some debugging to see if that codepath is actually being executed, and you could also ask Taras about it to see if it's some kind of expected Telemetry behavior?

::: browser/base/content/browser.js
@@ +8207,5 @@
> +    try {
> +      Services.telemetry.getHistogramById("FX_IDENTITY_POPUP_OPEN_MS").add(openingDuration);
> +    } catch (ex) {
> +      Components.utils.reportError("Unable to report telemetry for FX_IDENTITY_POPUP_OPEN_MS.");
> +    }

for completeness, should null out _popupOpenTime here.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-02-02 02:11:57 PST
Created attachment 593769 [details] [diff] [review]
Patch for bug

I debugged my previous issue and it was because I added the new histogram in the middle of the others without doing a clobber build. In other words, the issue I was seeing won't actually happen in practice.

I talked with Taras and Nathan Froyd about the strings being used here and they asked that the description strings mention the units of measurement for time deltas. They also said that the data association of preexisting telemetry histograms will not break if only the strings are updated.
Comment 4 Dietrich Ayala (:dietrich) 2012-02-02 02:58:30 PST
Comment on attachment 593769 [details] [diff] [review]
Patch for bug

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

looks good, r=me. thanks!
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-02-02 03:08:10 PST
Thanks Dietrich!

https://hg.mozilla.org/integration/fx-team/rev/5f3eedc770fa
Comment 6 Tim Taubert [:ttaubert] 2012-02-04 02:35:27 PST
https://hg.mozilla.org/mozilla-central/rev/5f3eedc770fa

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