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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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)]

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)]

Attachment: 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)]

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)]

Thanks Dietrich!

https://hg.mozilla.org/integration/fx-team/rev/5f3eedc770fa

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5f3eedc770fa