Closed Bug 723090 Opened 8 years ago Closed 8 years ago

Add telemetry probe for the opening time of the site identity popup

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Summary: Add telemetry probe for the opening time of the site identity block → Add telemetry probe for the opening time of the site identity popup
Attached patch Patch for bug 723090 (obsolete) — Splinter Review
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?
Attachment #593458 - Flags: feedback?(dietrich)
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.
Attachment #593458 - Flags: feedback?(dietrich)
Attached patch Patch for bugSplinter Review
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.
Attachment #593458 - Attachment is obsolete: true
Attachment #593769 - Flags: review?(dietrich)
Comment on attachment 593769 [details] [diff] [review]
Patch for bug

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

looks good, r=me. thanks!
Attachment #593769 - Flags: review?(dietrich) → review+
https://hg.mozilla.org/mozilla-central/rev/5f3eedc770fa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.