1937186 - Determine how to migrate histograms that clear themselves in non-test code: PWMGR_* in _gatherTelemetry()

Status: RESOLVED FIXED

(Toolkit :: Password Manager, task, P1)

Description

[Chris H-C :chutten]

In our effort to migrate histograms to use Glean APIs in bug 1935420 we've found some uses of the JS per-histogram clear() API in non-test contexts. We're not entirely certain what to do about these uses without the aid of the people who own them.

For this bug we're specifically looking at the histograms being written to in _gatherTelemetry() in LoginManager.sys.mjs:

• PWMGR_BLOCKLIST_NUM_SITES
• PWMGR_NUM_SAVED_PASSWORDS
• PWMGR_NUM_HTTPAUTH_PASSWORDS
• PWMGR_SAVING_ENABLED
• PWMGR_USERNAME_PRESENT
• PWMGR_LOGIN_LAST_USED_DAYS
• PWMGR_NUM_PASSWORDS_PER_HOSTNAME

Some questions:

• Are the data generated by these instrumentation in use?
  • Because if they're not, we can just remove them and be on our merry way.
• Can we encode this data in a different way to continue satisfying the same uses, or is it important that they all e.g. remain histograms despite holding single values?
  • If we can, then we'll move these to Scalars if you'd like them to remain in Legacy Telemetry, or just to Glean if you don't need them in there any more.
  • We'll need to decide about PWMGR_NUM_PASSWORDS_PER_HOSTNAME independently since it does actually record multiple values, albeit in a very non-histogram way -- if its data is important to keep static independent of the frequency that _gatherTelemetry is called, we've got some work ahead of us.

:mkmelin, as someone who has touched this file somewhat recently, would you happen to know the answers to these questions? Or to whom we should instead direct them?

Comment 1

[Magnus Melin [:mkmelin]]

Unfortunately I know very little about it. I don't think Thunderbird cares about this telemetry data.

Passing the buck (wild guess)

Comment 2

[Micah [:mtigley] (she/her)]

Thanks! Let me check with the team to see what we want to do with this telemetry.

One question: Chris do you know when and how often this idle-daily notification is dispatched to the main thread?

Comment 3

[Chris H-C :chutten]

"idle-daily" is notified the first 3min-wide idle period that is 24h after the previous "idle-daily" notification, at least according to my reading of this code.

(( Now, I did also in looking this up just find that DiscoveryStream might also notify on its own recognizance for some reason? I haven't found anything that actually triggers that, so maybe this is dead code. But still. Wow. ))

So, in full, "gather-telemetry" should happen once a day, tops. But also note the comment: it shouldn't matter overmuch how frequently it happens because it'll stop happening at all as soon as bug 1127907 lands. Which I'm sure will be any decade now.

Comment 4

[Chris H-C :chutten]

:mtigley in conversation on Slack agrees that we could delete the existing probes entirely so long as we still report the number of passwords somewhere, especially if solely in Glean.

I hope to have a patch that does this up soonish.

Comment 5

[Chris H-C :chutten]

Attachment: Bug 1937186 - Migrate PWMGR_* histograms relying on clear() r?dimi,mtigley

PWMGR_SAVING_ENABLED and PWMGR_NUM_SAVED_PASSWORDS are removed,
with Glean alternatives implemented. The new metrics always have the correct
value and will report them in every "metrics" ping.

PWMGR_BLOCKLIST_NUM_SITES, PWMGR_NUM_HTTPAUTH_PASSWORDS,
PWMGR_USERNAME_PRESENT, PWMGR_LOGIN_LAST_USED_DAYS, and
PWMGR_NUM_PASSWORDS_PER_HOSTNAME are merely removed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/525ebaf50f43
Migrate PWMGR_* histograms relying on clear() r=mtigley

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34e5c0e09e46
Backed out changeset 525ebaf50f43 for causing mass failures. CLOSED TREE

Comment 8

[agoloman]

Backed out for causing causing mass failures.

• Backout link
• Push with failures
• Failure Log

Comment 9

[Chris H-C :chutten]

Oh, LoginHelper is used on child processes? Okay, should just be a one-line fix.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7131829268b5
Migrate PWMGR_* histograms relying on clear() r=dimi,mtigley

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7131829268b5