Strange, I thought we had another bug filed for Thunderbird telemetry in general. Wayne, am I missing it? Regardless of this specific data point we'd like to be able to use telemetry in Thunderbird, we can either file a new bug in Thunderbird land or just morph this one since it already contains some info.

Looking at the internals link you mentioned earlier it seems that all we may be missing is setting datareporting.healthreport.uploadEnabled to true. We already have toolkit.telemetry.unified set to true.

I'm not sure how to differentiate saved-session pings from main pings from a code perspective, are there some functions that would send the one or the other? Would it be fair to assume that all code related to sending telemetry pings is in toolkit?

Georg's out on PTO, but I can answer some questions.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)

Looking at the internals link you mentioned earlier it seems that all we may be missing is setting datareporting.healthreport.uploadEnabled to true. We already have toolkit.telemetry.unified set to true.

That might do the trick. There are some other policy-related things you'll need as well (ensuring your Privacy Notice covers the collections, allowing the user to view the Privacy Notice for some period of time before sending initial Telemetry (see TelemetryReportingPolicy.jsm), ensuring the user has UI to opt out of collections, ...), and there might be some interesting corner cases (I don't know if Thunderbird builds pingsender, for instance), but flipping the pref is certainly necessary if not sufficient.

I'm not sure how to differentiate saved-session pings from main pings from a code perspective, are there some functions that would send the one or the other? Would it be fair to assume that all code related to sending telemetry pings is in toolkit?

With toolkit.telemetry.unified set to true there are no "saved-session" pings any more, just "main" pings. (This is as of last April or so since bug 1443603).

All Firefox Desktop Telemetry ping assembly and sending code is in toolkit/components/telemetry. Custom pings ("uitour-tag", "sync", others) may live elsewhere, and underpinning tech (like the XHR we use to POST the ping and ipc we use to collate accumulations) lives wherever that stuff lives, but the central pings are there

This sounds great, thanks for the info! I seem to recall we've had a notification for telemetry before, so maybe it isn't very much work to revive and extend that. We'll also take a look to see if our privacy policy covers it. Magnus, can you prioritize this accordingly and have someone work on it?

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)

Strange, I thought we had another bug filed for Thunderbird telemetry in general. Wayne, am I missing it? Regardless of this specific data point we'd like to be able to use telemetry in Thunderbird, we can either file a new bug in Thunderbird land or just morph this one since it already contains some info.

I am not aware of any open bug reports. The most "recent" history of bugs is
fixed Bug 956060 - Fix missing raw Thunderbird telemetry data
fixed Bug 956101 - Thunderbird data is missing from telemetry.mozilla.org
https://github.com/mozilla/telemetry-dashboard/issues/16 Telemetry Dashboard no longer supports Thunderbird

Note, "support" for plugins was dropped in Thunderbird Bug 1508942 - Remove all support for plugins (applets, etc.)

I've had a dig through the telemetry system, and written up some notes. I haven't really looked at the server/analysis side of things - I've just been catching data locally and figuring out how to start adding telemetry probes to TB.

Here are my notes:

Telemetry in Thunderbird

Docs

The Telemetry doc index is at:

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/index.html

There's a good summary of settings (both compiletime and runtime prefs):

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/internals/preferences.html

Compiling

Telemetry is not compiled in by default. You need to add the following line
to your mozconfig:

export MOZ_TELEMETRY_REPORTING=1

The nightly and release configs have this setting already ($ grep -r MOZ_TELEMETRY_ mail/config/mozconfigs).

Prefs

There's a complex set of conditions to set up telemetry reporting.
The runtime settings needed for a minimal test setup are:

• toolkit.telemetry.server - URL where the collected data will be POSTed to
  (https://incoming.telemetry.mozilla.org). So if you're running a local
  server for testing, you'll likely want this to be some localhost URL.
• toolkit.telemetry.server.owner - The owner of the server (Mozilla).
  The implication is that it's polite to change this if you're running a
  non-Mozilla server.
• toolkit.telemetry.send.overrideOfficialCheck - usually, telemetry is only
  send for official builds (ie export MOZILLA_OFFICIAL=1 in mozconfig).
  Setting this to true enables sending for unofficial builds.
• datareporting.policy.dataSubmissionEnabled - allows submission to the
  server.
• datareporting.policy.dataSubmissionPolicyBypassNotification - bypasses the
  checks to see if the policy has been shown and agreed to by the user. Set it
  to true for testing.
• toolkit.telemetry.log.level - very handy for watching telemetry activity in the javascript console.Trace,Debug,Info,Warn`, etc...

eg paste into your prefs.js:

user_pref("toolkit.telemetry.server", "http://localhost:8080/wibble");
user_pref("toolkit.telemetry.server_owner", "Nobody");
user_pref("datareporting.policy.dataSubmissionPolicyBypassNotification",true);
user_pref("datareporting.policy.dataSubmissionEnabled", true);
user_pref("toolkit.telemetry.log.level", "Trace");
user_pref("toolkit.telemetry.send.overrideOfficialCheck", true);

TODO: need to check the default prefs included in builds to see if there
are any telemetry-related changes required (both for testing and for release
builds).

Data-collection Policy

It's expected that the user will have been shown and agreed to the data
collection policy. For testing we can bypass this via
datareporting.policy.dataSubmissionPolicyBypassNotification
but there are a bunch of settings which track which version of the policy
the user has agreed to.

I haven't looked into the UI side here. I'd guess a bit of work needs to be
done to show/update/record policy info, using the firefox UX as a template.

Debugging

Running a test server

To run a test server locally to dump out the sent data, try
https://github.com/mozilla/gzipServer
(or alternatively https://github.com/bcampbell/webhole).

Make sure you set toolkit.telemetry.server/toolkit.telemetry.server_owner
to point to your local server.

Log output

If you've got logging on (eg user_pref("toolkit.telemetry.log.level", "Trace");),
the output will show up on the javascript console:

Menu => "Tools" => "Developer Tools" => "Error Console"

If data isn't showing up, keep an eye out for messages in the console.
For example: "Telemetry is not allowed to send pings" is an indication that
the official-build check is failing (overridden by
toolkit.telemetry.send.overrideOfficialCheck).

Test pings

From the javascript console, you can force an immediate test ping:

Cu.import("resource://gre/modules/TelemetrySession.jsm");
TelemetrySession.testPing()

Adding a telemetry probe

The types of data collection are outlined here:

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/start/adding-a-new-probe.html#rich-data-aggregate-data

The probe definitions are contained in .yaml and .json files under toolkit/components/telemetry (Scalars.yaml, Events.yaml etc).
Of course, these probes are all specific to Firefox.

The definitions are set at build time, using a bunch of python scripts in
toolkit/components/telemetry/build_scripts to generate the C++ files which
define the probe resistry (enums, string tables etc).

The code-generation scripts look like they can handle multiple probe definition files,
so we should be able to add extra .yaml/.json files into the mix with our new probes.
But currently the build process doesn't allow for invoking the build scripts with
multiple arguments, so we'd need at least a small M-C patch to properly support extra
Thunderbird-specific probes.

We'd need a way to hook toolkit/components/telemetry/moz.build to slip extra
files into the histogram_files, scalar_files etc... lists. Not quite sure
what the best way to do this is.

There may be some serverside considerations - if the probe definitions are used also
by the telemetry server (I'm unsure) we need to make sure that's accounted for on the
server setup.

Custom pings

For now, the most accessible way to add telemetry is to use a custom ping, which
doesn't require any build-time setup. See:

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/custom-pings.html

Other considerations

We probably need some coordination on what data is to be collected.
Perhaps a lightweight version of the
firefox process?

Do we need to worry about telemetry accidentally being sent out during
locally-run xpcshell tests? (I'd guess no. In the worst case it might
throw some rubbish at your local test server, but a default profile
won't be set up with all the policy acceptance prefs required to hit a
real live server).

Should we use a "tb-" or "mail-" prefix on TB-specific probe names to avoid
clashes?

Just a hacky little example of adding a custom ping (via submitExternalPing()). It's on the feeds code - whenever a feed is downloaded it pings out the feed type (feedType:rss/atom/etc) and the number of items found (nItems).

It'll send out something like this:

{"type":"tb-feed-parsed","id":"894174c5-93d2-4500-9a49-2518b4c36a66","creationDate":"2019-04-29T02:02:29.863Z","version":4,"application":{"architecture":"x86-64","buildId":"20190425081709","name":"Thunderbird","version":"68.0a1","displayVersion":"68.0a1","vendor":"","platformVersion":"68.0a1","xpcomAbi":"x86_64-gcc3","channel":"default"},"payload":{"feedType":"ATOM_IETF","nItems":10}}

I think the next thing to do is try and test adding some thunderbird-specific build-time-defined probes, and make sure they show up in a useful form in the analysis tools. I'm a little concerned about polluting the live server with accidental rubbish though...

Probably also a good time to start thinking up questions we'd like to see answered by telemetry. I think that should be what drives the metrics we choose to collect.

(In reply to Ben Campbell from comment #13)

Probably also a good time to start thinking up questions we'd like to see answered by telemetry. I think that should be what drives the metrics we choose to collect.

A hearty +1 to that. A questions-first approach often leads to the leanest data collection.

Speaking of which, Mozilla also published https://leandatapractices.org to help organizations construct their data collection practices. It might be of help.

This patch is a little test run of defining a new thunderbird-specific Scalar probe and getting it included into the telemetry code generation. This is the C-C part, and requires the corresponding M-C patch too.

... and here's the corresponding M-C patch. It's currently hardcoded to look for our custom defintions in comm/mail/components/telemetry/Scalars_tb.yaml, but I think some sort of CONFIG setting could work (see example in the patch) if I can work out where to set it.

Further thoughts:

1. it looks like it's possible to slip in additional TB-specific probe definitions to the telemetry system (albeit with some changes to the M-C code generation, which we need to keep as unobtrusive as possible).

2. Does the server/analysis dashboard side rely on the probe definitions in toolkits/components/telemetry? Seeing the description fields in https://telemetry.mozilla.org/probe-dictionary seems to imply that it does. How does the server get hold of these?

3. adding extra probes is likely to alter the internal enum values in the C++ code. Does the server side ever rely on this numbering, or is it considered purely internal to the C++? (I can't imagine the numbering is important - it'll change all the time. I'd guess that it's only ever strings that go over the wire, but worth checking!)

Hey, another Telemetry team member here.

1. Maybe we can work something out and make it more easy to allow additional files in m-c, so that the code bases don't need to diverge too much. I think we'd be ok with taking patches to make that happen
2. The probe-scraper reads the same definition files from m-c and other places. If I'm not mistaken so far it's mostly an informational service (like exposing it in the probe-dictionary) and nothing for the analysis depends on that, in the future our backend might depend a bit more on the definitions
3. Nope, enum values are purely internal, to the outside everything is identified by its name.

(In reply to Jan-Erik Rediger [:janerik] from comment #18)

Hey, another Telemetry team member here.

1. Maybe we can work something out and make it more easy to allow additional files in m-c, so that the code bases don't need to diverge too much. I think we'd be ok with taking patches to make that happen

Great - I'm hoping to come up with something as trivial and unobtrusive as possible! (the M-C patch above being a rough first check to see if it's feasible to hook in extra definition files).

2. The probe-scraper reads the same definition files from m-c and other places. If I'm not mistaken so far it's mostly an informational service (like exposing it in the probe-dictionary) and nothing for the analysis depends on that, in the future our backend might depend a bit more on the definitions

That's good to know. I think the idea would be to run a new instance of the existing backend, with as little change as possible.
(and I had an idea parse_scalars.py et al might show up again somewhere ;-) - they're in the M-C code-generation, with copies in probe-scraper, it looks like).

3. Nope, enum values are purely internal, to the outside everything is identified by its name.

Wonderful, that all sounds very promising - thanks for jumping in!

One extra note is that the aggregator that powers telemetry.mozilla.org's Measurement Dashboard soaks the definitions files (Histograms.json, Scalars.yaml, Events.yaml) directly from hg.mozilla.org. Its successor (planned for later this year) will likely use the probe-scraper, but for now these paths are hardcoded there.

This should only matter if you were hoping to have the aggregator aggregate your metrics, too. If not, then having them be in a separate area and brought in at build time may actually be the preferable solution.

Would it help to organize a chat or meeting of some kind? We might have some helpful tips or near-future plans that could help out. Or at the very least we confirm things like how you'll definitely need to include all the m-c scalars, events, and histograms definitions (like you're doing already in your test patches) otherwise things'll start error-ing out pretty hard.

Sounds like we should indeed have a meeting! Ben, sancus (see cc) could attend from Thunderbird, and I'm interested as well (but timezone diff may it inconvenient). I think Ben is UTC+13, sancus UTC-7 and I'm UTC +3. Are you able to suggest a time?

Also if there is a main up-to-date documentation page about telemetry somewhere that could be useful reading for us.

A meeting would be excellent. We've taken scheduling to email.

As for documentation, the in-tree docs might be the most relevant for the client portions: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/index.html

This might be less relevant, but we also document the systems and datasets that the client-side collections support: https://docs.telemetry.mozilla.org/

You can find us on irc.mozilla.org (for now) at #telemetry if you'd like some more synchronous ways to chat.

Just some notes on the probe parsers:

Bug 1282098 talks about moving them out of M-C... but I think the consensus was to leave the canonical source in M-C, and have a PyPi package which tracks it (for the data team, which need access to the probe definitions but won't necessarily have M-C).

probe-scraper has it's own copy of the probe parsers, with some changes that haven't yet been merged (see https://github.com/mozilla/probe-scraper/issues/6)

OK, so here's a more fully-formed attempt at patching M-C to handle extra probe definition files.

Firstly, it patches the various code-generation scripts so they can load/merge definitions from multiple files.

Secondly, it alters the telemetry moz.build to check config vars for extra probe definition files:
MOZ_TELEMETRY_EXTRA_SCALAR_FILES
MOZ_TELEMETRY_EXTRA_HISTOGRAM_FILES
MOZ_TELEMETRY_EXTRA_EVENT_FILES

(and those vars can be set in the top-level TB moz.configure using set_config())

I'm not totally happy with this approach, but it's the least-intrusive solution I've come up with. If anyone has a better solution, I'm all ears!
Failing that, I'll try and knock it into a proper proposal for applying to M-C. I suspect it might need breaking up - the mozparser module changes might need to be handled separately.

(In reply to Ben Campbell from comment #23)

Just some notes on the probe parsers:

Bug 1282098 talks about moving them out of M-C... but I think the consensus was to leave the canonical source in M-C, and have a PyPi package which tracks it (for the data team, which need access to the probe definitions but won't necessarily have M-C).

Yes, that's true. The canonical source lives in m-c, and is mirrored to PyPi for easier access by other teams.

probe-scraper has it's own copy of the probe parsers, with some changes that haven't yet been merged (see https://github.com/mozilla/probe-scraper/issues/6)

Yes, they are slightly diverging for reasons™, but there's a not too concrete plan of getting it to rely on the PyPi package only.

Here's a test that fails if the Thunderbird probes aren't included in the build. I've also included the two existing Thunderbird probes that are still referenced by the code.

You're right, adding the file would've been helpful! :-/

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:benc, could you have a look please?
For more information, please visit auto_nag documentation.

We know. Quite busy right now.

Some things that could be useful to collect:

• number of accounts + types. server.isGMailServer too
• size of each mail account (in KB)
• filelink usage: size sent, times not used even though size was above threshold
• number of calendars + types
• startup time
• system architecture and distributor/packager (for linux) [maybe already available in data]
• number of enabled add-ons (excluding system ones) [maybe already available in data]
• address books: number of address books, type, number of contacts
• chat account type details
• account setup: success/failure
• mail volumes: how many mails sent
• mail volumes: how many mails opened
• S/MIME:
  • signed: how many opened + how many sent.
  • encrypted: how many opened + how many sent.
  • has identity with encrypt by default set to true
• filter usage: has filters set up
• toolbar custimization: has customized toolbar and which ones
• calendar tasks: using it?
• has identity set to plain text compositon

Rebased version of the M-C patch to add support for extra probe definitions (basically the same as the one already reviewed in phabricator).

@chutten - if you're still happy with this patch, what are the next steps to get it landed in M-C?

This patch adds "thunderbird" as a supported product in the now-mandatory products field in the probe definitions. This seems like the obvious approach, but I'm open to better suggestions that don't fiddle in M-C :- )
I can imagine the products field being quite handy - TB telemetry can ignore all the firefox-specific probes (or add "thunderbird" to any of them we do want to collect).

An updated version of the patch with the new CC probe definition files.
Relies on both the MC patches being applied first.

The new CC telemetry unit tests all seem to pass fine for me locally. But I just realised I've no idea how to run a try build with changesets for both M-C and C-C... I guess we need to get the M-C ones landed first?

Geoff: flagging you up just in case you can think of anything specific I've missed.

(In reply to Ben Campbell from comment #36)

@chutten - if you're still happy with this patch, what are the next steps to get it landed in M-C?

Marking the bug with the checkin-needed keyword will ask the Sheriffs to land the two m-c patches for us. (If we'd done the reviews in Phabricator, I'd task the Lando service to do it).

Do you want to do that?

https://hg.mozilla.org/mozilla-central/rev/bf126cb4b08f
https://hg.mozilla.org/mozilla-central/rev/6b90caa175cb

Hmm, looks like landing the M-C parts in this range broke TB big time:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1125aad05d5c39c2453c04ca6d037afaef16c296&tochange=f6a1009171e521c4917dcaccf69aba9093467e99

I'm not really pleased for two reason:

• apparently this was landed on M-C without any try run.
• I wasn't CC'ed on the bug, so I came to find this bustage on a Sunday at lunchtime

So what's the way forward here? This should be backed out from M-C until it's ready, right?

Talked to Jorg on IRC - agreed that backing out the patches best for now.
I suspect it might have been the product-field patch which did it (from the test point of view suddenly the app was "thunderbird" and all the firefox probes have disappeared!). Will check it out and report back tomorrow.

(In reply to Jorg K (GMT+2) from comment #46)

Hmm, looks like landing the M-C parts in this range broke TB big time:

Well only the telemetry tests, apparently? That they are broken really doesn't mean a thing yet since Thunderbird doesn't yet do telemetry.

So what's the way forward here? This should be backed out from M-C until it's ready, right?

I think we should hold off and fix the errors. Broken telemetry tests when we know we're not using telemetry is not a problem. Setting MOZ_TELEMETRY_REPORTING=0 could perhaps help in the meantime (I'll have to check)

There are an estimated 200 test broken with all X's red on some platforms. This makes sheriffing impossible and it's hard to tell whether something else caused bustage, here an excerpt:
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_corrupt_database.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_corrupt_database.js | test_corrupt_databases_get_reported_and_blown_away - [test_corrupt_databases_get_reported_and_blown_away : 108] 3 == 2
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_index_bad_messages.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_index_compaction.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_index_messages_imap_online.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_index_messages_imap_online_to_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_migration.js | xpcshell return code: 0

Many failures also in security/manager, toolkit/components/, toolkit/profile/, etc.

Let's find out whether it was this bug that broke the tests.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5addad118939713713ea9efe4592527223c3ccbd

For the gloda failures to be fixed, we need the c-c part of this patch to land since those probes now no longer exist.

Sure, but the C-C parts isn't ready to land. So in general, you get all parts ready, you do a try run with all parts, then you land the M-C parts and at the same time notify the C-C sheriff to land the C-C part when M-C merges. Which part of the process did you adhere to here?

https://hg.mozilla.org/mozilla-central/rev/dcd9ee807647b01a5bf493ad0c8e11cfb5c17e8b
Backed out 2 changesets (bug 1427877) for causing Thunderbird xpcshell-bustages. a=backout

Thanks for the try in comment #50, I was going to do one myself. Before the backout I checked some tests locally, including picking toolkit/profile/xpcshell/test_check_backup.js at random.

For further action here, please adhere to that I said in comment #52. Try runs are cheaper than having things backed out from M-C.

And I don't think landing the C-C part as it is will help anyway. It's the firefox tests which are failing, and I think it's because of the second M-C patch (it changes the product from "firefox" to "thunderbird", so suddenly all the firefox probes disappear and the tests fail).
Off the top of my head, I think the options are:

1. don't treat thunderbird as a separate product from a telemetry POV - ie pretend to be firefox
   pro:

• less code intrusive
  con:
• smells bad (it obviously is a different product)
• TB telemetry collects all the firefox probes, which we probably don't want.

2. TB is a separate product
   We can add thunderbird as a product for any M-C probes we do want (probes have a list of products they are valid for).
   Still likely to be a bunch of probes which are firefox-only, which need some handling in the M-C unit tests. Maybe we have to early-out whole tests if product is "thunderbird".
   pro:

• The Right Thing (tm) - separate product, and we can control what data gets collected.
  con:
• More intrusive. More M-C patching.

Geoff's try is green apart from one which appears(?) to be intermittent (although I've never seen it before):
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_startup.js | test_modify - [test_modify : 48] "[\"addon2@tests.mozilla.org\"]" == "[]"

BTW, try runs including M-C patches are very easy to do: Push the M-C patches to M-C try without any try syntax, then reference the try changeset in C-C's .gecko_rev.yml. Example in comment #50:
https://hg.mozilla.org/try-comm-central/rev/5addad118939713713ea9efe4592527223c3ccbd

-GECKO_HEAD_REPOSITORY: https://hg.mozilla.org/mozilla-central
-GECKO_HEAD_REF: default
+GECKO_HEAD_REPOSITORY: https://hg.mozilla.org/try
+GECKO_HEAD_REF: 85a003a8633c61b6b1371161eb4cc72e69102d09

which is based on
https://hg.mozilla.org/try/graph/85a003a8633c61b6b1371161eb4cc72e69102d09 (using graph to see the backout).

Documentation here (which doesn't say anything else):
https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer#Pushing_mozilla-central_patches

I think I've got it all sorted out now. As I suspected, it was adding "thunderbird" as a possible telemetry "product" field which broke loads of M-C tests: a whole heap of the probes were effectively disabled when running as thunderbird, and thus any unit tests relying on them would fail.
I've been through all the failing tests and added "thunderbird" to the probes they rely on, and the following patches will reflect this.

I took the approach of only adding "thunderbird" to the bare minimum of probes - I figured it'd be easier to explicitly choose which extra M-C probes we want to add (and there will be a bunch, especially regarding memory usage and performance). If we just added all of them, it'll be an arse to later on figure out which ones are safe to remove.
One exception was the telemetry-related telemetry probes - I added most of those so we can get feedback and diagnostics on the telemetry itself.

It was interesting to see how the unit tests used the telemetry. Mostly it was checks that particular telemetry probes were working as expected. But there were a few non-telemetry-related tests where various systems used probes to see if things had worked or failed, which I thought was interesting - some systems are more resistant to unit-testing that you'd like, and using the telemetry to gain some insight could be a nice approach, I think.
There were also a couple of places where M-C unit tests were making assumptions that unrelated events would be triggering, which caused problems on TB. Tedious to track down, but easy to fix once you know what's happening.

So. I've got about 4 patches to land (two in M-C and two in C-C), and I've tried to arrange them so they don't require any synchronised landing upon both M-C and C-C at once. Roughly, they'll be:

1. Small C-C patch to disable the existing TB telemetry probes while we transition
2. M-C patch to add support for extra definition files (same as what's already been reviewed and approved)
3. M-C patch to add "thunderbird" as a telemetry product, and to update all the M-C probe definitions and tests accordingly.
4. C-C patch to restore TB-specific probes and add some C-C unit tests.

This one needs to land first, in C-C.
Just disables the existing TB-specific probes (which are currently in the M-C definitions and will be moved and restored in a subsequent patch).

Oh. No "R?" flags here... Someone care to give it a review for me?

The first M-C patch. Depends on 1427877-cc-disable-tb-specific-probes.patch landing first in C-C.

Just an updated version of the already-reviewed patch to allow pulling extra probe definitions from other files (ie so we can define TB-specific probes).
No real code changes from the previous patch, but there's been a bit of churn, so a rebase seemed prudent.

(The second M-C patch, to land after 1427877-mc-allow-extra-probe-definitions-3.patch).

• Defines a new "thunderbird" product enum for telemetry.
• Adds it to the products field of a bunch of probes required to pass the unit tests.
• a couple of minor fixes where tests were relied on unrelated Events triggering (in test_LoginManagerParent_onGeneratedPasswordFilledOrEdited.js and test_TelemetryController.js).

Here's a C-C try job, with the two M-C patches applied (assuming I got my .gecko_rev.yml magic right):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=264508350&revision=71c6d11ded85ba99d57c8e4b96bcaba19ae42a09

Seems good so far... no more massive telemetry-related test fails this time...

Good to paste the try changeset as well, ....
https://hg.mozilla.org/try/graph/c0073f343c62f760f385c370f411f88196c826a7

Magnus, can you please review attachment 9089782 [details] [diff] [review].

At least for the m-c patches, might as well set up phabricator and to the reviews through there.

But why disable the current tb probes? As this will anyway have to land at the same time, why not just land a patch that uses the "new way" at that time?

(In reply to Magnus Melin [:mkmelin] from comment #66)

At least for the m-c patches, might as well set up phabricator and to the reviews through there.

But why disable the current tb probes? As this will anyway have to land at the same time, why not just land a patch that uses the "new way" at that time?

The idea was to decouple the M-C and C-C changes. The order is still important, but nothing relies on landing stuff in both trees simultaneously.
It sounds like simultaneous landing is doable, but after last time I figured it was best to err on the side of caution.

New plan: I'll leave the existing TB-specific probes in for the M-C patches (and submit them via phabricator). It means another M-C patch at some point to move them over, but we'll likely be submitting the odd patch anyway - to add thunderbird on some of the other M-C probes we'd like to receive.

Depends on D44425

Sancus: I've got an M-C patch up which adds thunderbird to a bunch of firefox-specific telemetry probes. Some are things we probably want anyway, but a lot are just so all the unit tests pass. We might be able to come up with a better solution for the tests, but I'm inclined to say we should just go with what we've got now, even if it means collecting some superfluous data.
But I figured you should be in the loop on this!
The patch and discussion are at:
https://phabricator.services.mozilla.com/D44426

Chris, would you be open to landing D44425 and D44426? As we discussed on D44426 it'd be nice to add a product-override to avoid adding TB to some of the probes just for test-passing purposes. But for now it'd be nice to just close the loop, get some data flowing and start getting a proper feel for it all.

Here's an update of Geoff's CC patch to add the extra TB-specific probe definitions, and some unit tests to ensure they're being built correctly.

Relies on D44425 and D44426 landing first.

(In reply to Ben Campbell from comment #71)

Chris, would you be open to landing D44425 and D44426? As we discussed on D44426 it'd be nice to add a product-override to avoid adding TB to some of the probes just for test-passing purposes. But for now it'd be nice to just close the loop, get some data flowing and start getting a proper feel for it all.

File me a follow up bug for the override and I'll set them to land on Monday : ) (getting close to my weekend at this point.)

Thanks Chris! The tyranny of the timezones strikes again, but here's a belated Bug 1579752.

Ah, poop. Looks like we need a rebase. Autoland can't land the patches on its own : |

see also Bug 702561 - Add telemetry reporting for calendar

(In reply to Chris H-C :chutten from comment #75)

Ah, poop. Looks like we need a rebase. Autoland can't land the patches on its own : |

OK, rebased patches submitted.
Handily, moz-phab seems to have identified that I was updating existing patches without any special input from me. Cool! And slightly terrifying - I'm not quite sure what the mechanism for that is...

(and sorry about the delay - was ill all last week!)

Glad you're feeling better.

Pretty sure moz-phab knows because commit messages are amended to include the Phabricator url of the review, and moz-phab chats with phab to make sure the state's okay for uploading. No magic here, as far as I know : )

So what do I need to land on C-C when this is merged to M-C in three hours?
The changeset from the try run?
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=264508350&revision=71c6d11ded85ba99d57c8e4b96bcaba19ae42a09

https://hg.mozilla.org/mozilla-central/rev/fa4c29369074
https://hg.mozilla.org/mozilla-central/rev/7e72c7602d04

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #80)

So what do I need to land on C-C when this is merged to M-C in three hours?
The changeset from the try run?
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=264508350&revision=71c6d11ded85ba99d57c8e4b96bcaba19ae42a09

No, the aim was to decouple the changes - the M-C changes can land without borking up C-C.
Attachment 9090921 [details] [diff] is the C-C patch which hooks in some TB-specific telemetry definitions, but I wanted to get the M-C changes in first.

And now the M-C patches have landed, here's an update of the basics on the C-C side (a few nit fixes, added some more notes).

magnus: no R? on this bug, hence the Feedback? flag.

(In reply to Magnus Melin [:mkmelin] from comment #84)

Though the language could be changed a bit, the code generation happens...
where exactly? "happens build time"?

The README was a bit of an afterthought I've fleshed it out a bit now, so hopefully it should be more generally useful.

@@ +21,5 @@

+For Histograms, we use a TB_ or TELEMETRY_TEST_TB_ prefix.
+
+(Why not just TB_? Because the telemetry test helper functions
+getSnapshotForHistograms()/getSnapshotForKeyedHistograms() have an option
+to filter out histograms with a TELEMETRY_TEST_ prefix).

Maybe it would be better to change that filtering to just check if the name
includes that?

Probably, but I figured it was easiest to avoid an extra M-C patch right now. I only discovered it yesterday and just wanted to make sure it was noted somewhere.

Magnus, if you're OK with this patch can we R+ it?

Try build here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1094deed1b554397d8d78b09a0c55da9604d7d2e

Yeah looks ok. The try had bad luck, I sent off another one now https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=406e11b425eb0135b6246e7d4d11c75e0e0dfdd1

Please land attachment 9093773 [details] [diff] [review]

Done here?

(In reply to Jorg K (GMT+2) from comment #89)

Done here?

I'm happy to close this one. There are the probes Magnus suggested in Comment 35, but I'd suggest we open individual bugs for each requested probe to better debate and track them.

Yeah let's add more probes and other work in other bugs.