1183319 - add telemetry probe in about:logins for load time of getAllLogins() in the wild

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

For an easy example:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#639

Comment 2

[Allison Naaktgeboren :ally]

likely targeting Firefox 55 as the expiration date.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: telemetryStopWatchGetAllLogins

Vlad for data review, mfinkle for general android review.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8636830 [details] [diff] [review]
telemetryStopWatchGetAllLogins

I'm assuming that the optional "cpp_guard" is not needed here. We only seem to use it for probes that originate in Java.

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8636830 [details] [diff] [review]
telemetryStopWatchGetAllLogins

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

::: toolkit/components/telemetry/Histograms.json
@@ +8180,5 @@
> +  "PWMGR_ABOUT_LOGINS_GET_ALL_LOGINS_MS": {
> +    "expires_in_version": "55",
> +    "kind": "exponential",
> +    "high": 60000,
> +    "n_buckets": 30,

btw, we now have a page that previews what your histogram buckets will look like:

http://development.telemetry-dashboard.divshot.io/histogram-simulator/#low=1&high=60000&n_buckets=30&kind=exponential&generate=log-normal

We'll move it to the official telemetry.mozilla.org page in a couple of days

Comment 6

[Allison Naaktgeboren :ally]

Nifty thats handy.


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

Comment 7

[Allison Naaktgeboren :ally]

I'm about to be backed out for busting b2g. mfinkle, the cpp guard business wouldnt have anything to do with b2g would it?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #7)
> I'm about to be backed out for busting b2g. mfinkle, the cpp guard business
> wouldnt have anything to do with b2g would it?

No idea. Worth adding it and pushing to Try.

Comment 9

[Allison Naaktgeboren :ally]

What was my plan, but I thought it worth inquiring, like that time I accidentally broke SeaMonkey by including a symc dtd in a new place

Comment 10

[Allison Naaktgeboren :ally]

This was backed out.

green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d062e05007e2

Comment 11

[Allison Naaktgeboren :ally]

full green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2afd47fbe83d

Comment 12

[Allison Naaktgeboren :ally]

Attachment: aboutLoginsTelemetryStopwatch with idfef

see above for build run

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8638792 [details] [diff] [review]
aboutLoginsTelemetryStopwatch with idfef

Let's rollback to your original patch.

Let's look at the reason for the build bustage:

78[1G[J[34m 0:08.13(B[m File "/home/worker/workspace/gecko/toolkit/components/telemetry/histogram_tools.py", line 276, in from_Histograms_json
78[1G[J[34m 0:08.13(B[m raise BaseException, "error parsing histograms in %s: %s" % (filename, e.message)
78[1G[J[34m 0:08.13(B[m BaseException: error parsing histograms in /home/worker/workspace/gecko/toolkit/components/telemetry/Histograms.json: Expecting , delimiter: line 8233 column 1 (char 301995)

It seems like Histograms.json has a bad delimiter.

Looking at your patch though, things are fine. But when we look at what was landed, I see an extra leading "+" in Histograms.json that somehow slipped in:
https://hg.mozilla.org/integration/fx-team/rev/a6bf83d92196

I also see the "logins = Services.logins.getAllLogins();" line is indented a space too much, also not in the patch.

Adding the #ifdefs won't actually fix the build bustage, which kinda makes sense since aboutLogins.js is not included in b2g builds. But Histograms.json is included.

The original patch is simpler and cleaner, and should work fine as long as it's landed cleanly.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fc68f926b45

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

I see we have some data coming in now:
http://mzl.la/1IS6CDB

Mean: ~50ms, but I'm surprised that 75th %-ile is almost 1sec. Still, overall the majority of people are not seeing a large delay from the API.

I still question why we are using images/favicons though. Nearly all of mine are the default image.

Comment 16

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> I see we have some data coming in now:
> http://mzl.la/1IS6CDB
> 
> Mean: ~50ms, but I'm surprised that 75th %-ile is almost 1sec. Still,
> overall the majority of people are not seeing a large delay from the API.
> 
> I still question why we are using images/favicons though. Nearly all of mine
> are the default image.

Do we have an open bug filed on that? We should discuss the UX trade-offs there. We're probably suffering from the same sync issue that we have with desktop history items.

Comment 17

[Allison Naaktgeboren :ally]

open bug filed on default image favicons or for a trade off discussion? Either way I know of neither already on file but I can remedy that.