[Thunderbird Telemetry] collect number of accounts + types
Categories
(Thunderbird :: General, task)
Tracking
(thunderbird78+ fixed)
People
(Reporter: mkmelin, Assigned: rnons)
References
Details
Attachments
(1 file, 6 obsolete files)
6.38 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
It would be useful for getting a basic understanding of the standard setups people have to know how many accounts they have and the type of those accounts.
For chat, we could do im+protocol as a type.
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
This patch reports account types every time Thunderbird starts. For im accounts, use im_protocol
as
histogram bucket label. Also added a mochitest.
Assignee | ||
Comment 2•4 years ago
|
||
If in msgMail3PaneWindow.js
I write
#ifndef MOZ_SUITE
setTimeout(reportAccountTypes, 0);
#endif
Tests stuck and failed after a few minutes. Don't know why.
Assignee | ||
Comment 3•4 years ago
|
||
Learned about AppConstants, use
if (!AppConstants.MOZ_SUITE) {
setTimeout(reportAccountTypes, 0);
}
Reporter | ||
Comment 4•4 years ago
|
||
Comment on attachment 9150675 [details] [diff] [review] 1615981.patch Review of attachment 9150675 [details] [diff] [review]: ----------------------------------------------------------------- Maybe name it browser_accountTelemetry.js ::: mail/base/content/msgMail3PaneWindow.js @@ +790,5 @@ > } else { > window.setTimeout(loadStartFolder, 0, startFolderURI); > } > + > + if (!AppConstants.MOZ_SUITE) { All the things in mail/ are Thunderbird only, so this is not necessary. Things in mailnews/ are shared. Things in suite/ are Seamonkey only. @@ +801,5 @@ > + * histogram bucket label. > + */ > +function reportAccountTypes() { > + let histogram = Services.telemetry.getHistogramById("TB_ACCOUNT_TYPE"); > + MailServices.accounts.accounts.forEach(a => { for .. of is a bit more performant: for (let account of MailServices.accounts.accounts) ::: mail/components/telemetry/Histograms.json @@ +26,5 @@ > + "nntp", > + "exchange", > + "rss", > + "none", > + "im_facebook", Hmm, I don't know what we want to do with these no longer working types: facebook, skype, yahoo at least. And twitter is also permanently broken. We probably want to fiter them out from ever being sent out. ::: mail/test/browser/account/browser_telemetry.js @@ +94,5 @@ > + snapshot.values[ACCOUNT_TYPE_LABELS.indexOf("rss")], > + NUM_RSS, > + "RSS account number must be correct" > + ); > + would be good to add test cases for im types too, especially as that does some extra work
Reporter | ||
Updated•4 years ago
|
Comment 5•4 years ago
|
||
Hmm, I don't know what we want to do with these no longer working types:
facebook, skype, yahoo at least. And twitter is also permanently broken.
We probably want to fiter them out from ever being sent out.
Please do not do that -- we want to know if anyone is left using them so we can remove the code eventually.
It might make sense to report instant messaging accounts as a different metric for what it's worth. Isn't a huge deal though, should be easy enough to filter after the fact if we wanted.
Reporter | ||
Comment 6•4 years ago
•
|
||
(In reply to Patrick Cloke [:clokep] from comment #5)
Please do not do that -- we want to know if anyone is left using them so we can remove the code eventually.
Well they aren't working at all are they? With zero chance of coming back (potentially twitter).
Comment 7•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #6)
(In reply to Patrick Cloke [:clokep] from comment #5)
Please do not do that -- we want to know if anyone is left using them so we can remove the code eventually.
Well they aren't working at all are they? With zero chance of coming back.
Right, but we still have code that gives a message saying "Hey, this isn't working, remove the account". It'd be nice to get rid of that eventually. Besides I'd just recommend doing any filtering like that server side anyway.
Reporter | ||
Comment 8•4 years ago
|
||
Is there a reason we don't simply remove such accounts in a migration? It's gotta be many major versions since those worked.
Reporter | ||
Updated•4 years ago
|
Just knowing how many feed (type="rss") accounts there are is only moderately interesting. Each feed subscription is effectively a 'server', equivalent to 1 newsgroup in an nntp account or 1 email account. It would be more useful to know how many subscriptions there are per account, and how many folders per feed account, given that a folder may contain 0, 1, or many subscriptions ('servers').
Reporter | ||
Comment 10•4 years ago
|
||
True, but I think that should be another item to collect separately. It doesn't really fit the data model in here.
Assignee | ||
Comment 11•4 years ago
|
||
Fixed review comments except the not working im types. Will wait for further confirmation.
Assignee | ||
Comment 12•4 years ago
|
||
Forgot to add browser_accountTelemetry.js
Comment 13•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #8)
Is there a reason we don't simply remove such accounts in a migration? It's gotta be many major versions since those worked.
Yes, because we have no idea how many people are using those accounts. :) Regardless, that's a separate bug than this. (And so close to the next ESR I don't know why we'd remove them right now.)
The short version from my POV is: don't add more code to handle these accounts separately, just report all the IM accounts.
Reporter | ||
Comment 14•4 years ago
|
||
I still don't understand what you mean. We know how many people are using them: 0. They do not work, since many years. They can not use them. People can (and will) of course have the old account listed, but the account would not be able to do anything.
Anyway, let's ignore that in this bug. I've filed bug 1641763.
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 15•4 years ago
|
||
Comment on attachment 9151391 [details] [diff] [review] 1615981.patch Review of attachment 9151391 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin
Reporter | ||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a378b70d4431
Add telemetry to count account types. r=mkmelin
Comment 17•4 years ago
|
||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/cf52a9080e3f follow-up - Fix naming conflict that caused browser_tree.js to time out. rs=bustage-fix
Comment 18•4 years ago
|
||
I'm not sure if what I've just landed will also fix this failure that appeared on some Windows and Mac runs. I doubt it.
IMAP account number must be correct - "undefined" == 3 - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/account/browser_accountTelemetry.js :: test_account_types :: line 103
RSS account number must be correct - "undefined" == 1 - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/account/browser_accountTelemetry.js :: test_account_types :: line 108
IRC account number must be correct - "undefined" == 1 - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/account/browser_accountTelemetry.js :: test_account_types :: line 113
Assignee | ||
Comment 19•4 years ago
|
||
:darktrojan I think I need to rework on this bug. The landed patch used histogram.add
, which I guess will accumulate account numbers after restarting TB. There doesn't seem to be a histogram.set
api, so I need to use scalar probe instead.
Please comment out or remove browser_accountTelemetry.js
so that Treeherder can turn green, thanks.
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 20•4 years ago
|
||
I'll land a backout.
Comment 21•4 years ago
|
||
Backout by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/446a471a963d Backed out 2 changesets by request.
Assignee | ||
Comment 22•4 years ago
|
||
Changed to use keyed scalar probe instead of histogram probe.
Tests seem to pass on my Mac machine, will push to Try maybe tomorrow.
Assignee | ||
Comment 23•4 years ago
|
||
Lint.
Assignee | ||
Comment 24•4 years ago
|
||
Reporter | ||
Comment 25•4 years ago
|
||
Comment on attachment 9154069 [details] [diff] [review] 1615981.patch Review of attachment 9154069 [details] [diff] [review]: ----------------------------------------------------------------- Needs linting. (run ../mach eslint --fix mail/test/browser/account/browser_accountTelemetry.js ) ::: mail/base/content/msgMail3PaneWindow.js @@ +814,5 @@ > + im_odnoklassniki: 0, > + im_xmpp: 0, > + }; > + for (let account of MailServices.accounts.accounts) { > + let { type } = account.incomingServer; we typically use the below instead: let type = account.incomingServer.type
Assignee | ||
Comment 26•4 years ago
|
||
Thanks, lint was done in comment 23, I didn't trigger a new Try run.
Changed to let type = account.incomingServer.type
.
Reporter | ||
Comment 27•4 years ago
|
||
Comment on attachment 9154835 [details] [diff] [review] 1615981.patch Review of attachment 9154835 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin
Comment 28•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/49cb9cd70a75
follow-up - Use scalar probe to collect account numbers and types. r=mkmelin
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 29•4 years ago
|
||
Comment on attachment 9154835 [details] [diff] [review] 1615981.patch Telemetry addition - not risky.
Comment 30•4 years ago
|
||
Comment on attachment 9154835 [details] [diff] [review] 1615981.patch Approved for beta
Comment 31•4 years ago
|
||
bugherder uplift |
Thunderbird 78.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/0525eca6f121
Updated•4 years ago
|
Description
•