1899602 - Thunderbird Glean migrations

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Bug to track the actual migration of Telemetry probes to Glean.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - Lay the groundwork for enabling Glean and migrate tb.compose_format to Glean. r=babolivier

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate TB_COMPOSE_TYPE to glean. r=babolivier

See https://firefox-source-docs.mozilla.org/toolkit/components/glean/user/migration.html#discrete-distributions-kind-categorical-enumerated-or-boolean-use-glean-s-labeled-counter
I think the existing probe was wrong. It collected integers but labels were expected.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - convert tb.websearch.usage to Glean. r=vineet

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.account.count to glean. r=aleca

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.account.successful_email_account_setup to glean. r=aleca

Apparently this was not tested before.

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.account.failed_email_account_setup to glean. r=aleca

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.ui.interaction probes to glean. r=aleca

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.filelink.ignored to glean. r=aleca

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.uploaded_size.uploaded to glean. r=aleca

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.ui.configuration.folder_tree_modes to glean. r=#thunderbird-reviewers

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.ui.configuration.pane_visibility to glean. r=#thunderbird-reviewers

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.ui.configuration.message_header to glean. r=#thunderbird-reviewers

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.calendar.calendar_count to glean. r=aleca

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.calendar.read_only_calendar_count to glean. r=aleca

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.addressbook.addressbook_count to glean. r=#thunderbird-reviewers

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.addressbook.contact_count to glean. r=#thunderbird-reviewers

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.preferences_boolean to glean. r=aleca

Comment 18

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.preferences_integer to glean. r=aleca

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.mails.sent to glean. r=#thunderbird-reviewers

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.mails_read to glean. r=#thunderbird-reviewers

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.mails.read_secure to glean. r=#thunderbird-reviewers

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.mails.folder_opened to glean. r=aleca

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.account.oauth2_provider_count to glean. r=#thunderbird-reviewers

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - migrate tb.account.size_on_disk and tb.account.total_messages to glean. r=freaktechnik

Comment 25

[Magnus Melin [:mkmelin]]

:Dexter, :chutten - would be grateful for any feedback on these migrations

Comment 26

[Chris H-C :chutten]

Wow, that's 23 reviews. Any in particular you'd consider an exemplar of the whole set, or would you like eyes on all of them?

Comment 27

[Magnus Melin [:mkmelin]]

The most prominent classes are likely these:

We have a fair amount of labeled quantities (which isn't a type). I've used type: labeled_string for things like https://phabricator.services.mozilla.com/D213115 as advised on #glean
(Note for others, labeled_counter would have to be carefully set lifetime: application and tie to a once-in-a-run event - since counter can only add, not set, and we need to make sure to do it only once.)

Perhaps one other class that could use some more eyes is how I use event - there are a couple of these, like https://phabricator.services.mozilla.com/D213123 (the probe itself is not ideal and should be reworked in another bug)

Comment 28

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - remove obsolete Telemetry testing probes. r=#thunderbird-reviewers

Comment 29

[Chris H-C :chutten]

Both of them look just fine to me, with some small things to watch out for. Where this will become interesting is analysis: will you be able to answer the questions you have with data in these shapes.

Comment 30

[Magnus Melin [:mkmelin]]

One thing that came to mind is that using a "tb." prefix probably isn't the best idea now, since there can't be (future) metrics files for other folders - say for calendar/ - using the same prefix. I guess we should use "mail." for the ones in the mail/ folder

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c922bb4a746
Lay the groundwork for enabling Glean and migrate tb.compose_format to Glean. r=babolivier
https://hg.mozilla.org/comm-central/rev/d89ef19a9482
migrate TB_COMPOSE_TYPE to glean. r=babolivier
https://hg.mozilla.org/comm-central/rev/5953a605c975
convert tb.websearch.usage to Glean. r=vineet
https://hg.mozilla.org/comm-central/rev/7f0bfdbd6fc5
migrate tb.account.count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/20474f81f541
migrate tb.account.successful_email_account_setup to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/0375237a08a1
migrate tb.account.failed_email_account_setup to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/d9fa137dad19
migrate tb.ui.interaction probes to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/1b734c857eb8
migrate tb.filelink.ignored to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/5e81c59166a1
migrate tb.uploaded_size.uploaded to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/acb81094268b
migrate tb.ui.configuration.folder_tree_modes to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/3c7d205b96f5
migrate tb.ui.configuration.pane_visibility to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/333cf4f413c1
migrate tb.ui.configuration.message_header to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/34488e9695ba
migrate tb.calendar.calendar_count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/f55b66432f26
migrate tb.calendar.read_only_calendar_count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/6d081720ef95
migrate tb.addressbook.addressbook_count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/16fa84b786af
migrate tb.addressbook.contact_count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/01e21678b928
migrate tb.preferences_boolean to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/a4c13883c8fa
migrate tb.preferences_integer to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/4dd628be224b
migrate tb.mails.sent to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/4cf00dd97f7f
migrate tb.mails_read to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/6679f8d420f3
migrate tb.mails.read_secure to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/9d0300c2c6e4
migrate tb.mails.folder_opened to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/e8b9f19926e0
migrate tb.account.oauth2_provider_count to glean. r=aleca
https://hg.mozilla.org/comm-central/rev/27899ccf0430
migrate tb.account.size_on_disk and tb.account.total_messages to glean. r=freaktechnik
https://hg.mozilla.org/comm-central/rev/03745426540b
remove obsolete Telemetry testing probes. r=aleca

Comment 32

[Chris H-C :chutten]

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

One thing that came to mind is that using a "tb." prefix probably isn't the best idea now, since there can't be (future) metrics files for other folders - say for calendar/ - using the same prefix. I guess we should use "mail." for the ones in the mail/ folder

Oh yes, that's an excellent point. Especially since all datasets will be namespaced to the application id (which I assume will be thunderbird) so including tb. could be redundant. Sorry, I should've caught that naming smell when you asked for review.

Comment 33

[Chris H-C :chutten]

Oh, though I should mention, you can use the same category in multiple files. There's no restriction there. Though the pipeline will complain if you have a fully-duplicated metric name.

Comment 34

[Alessandro Castellani [:aleca]]

What are the next steps here?
Should we request 128 uplift?
Do you want to remove the tb. prefix?

Comment 35

[Magnus Melin [:mkmelin]]

I'll change/remove the prefix.
We should uplift to 128, but I'd wait until legal is finished so we can properly check data is getting sent correctly first.

:chutten, IIRC you mentioned earlier the Thunderbird app id also needs to be added to an allow list (in the data pipeline somewhere) so tb data wouldn't get discarded. Do you recall what that was, and how should that be handled?

Comment 36

[Chris H-C :chutten]

More than recalling it, we've documented it! https://mozilla.github.io/glean/book/user/adding-glean-to-your-project/index.html

I think the specific step you're after is 4, but the whole thing's a decent checklist to doublecheck.

Comment 37

[Magnus Melin [:mkmelin]]

Attachment: Bug 1899602 - Distribute metrics.yaml files to where they belong. r=aleca

Stop using the tb. prefix, and put the probes in component relative metrics.yaml files

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfb3e3498687
Distribute metrics.yaml files to where they belong. r=aleca

Comment 39

[Wayne Mery (:wsmwk)]

Candidate then for 128.1.0? Or do you have more coming?

Comment 40

[Ben Wu [:benwu]]

We've been receiving telemetry for this in the data pipeline for a few weeks now but it's been coming with the firefox-desktop app namespace so it's all landing in bigquery right now (but not in the correct columns because the metrics weren't added to probe-scraper). I want to check if this is intended or possibly side effect of reusing firefox code. Reading the conversation above, it sounds like this isn't intended.

Comment 41

[Alessandro Castellani [:aleca]]

Thanks for the report.
This definitely wasn't the intended outcome.
Alessio, can you advice on the steps to take to fix this? Is following the steps to add the probe-scraper all we need?

Comment 42

[Alessio Placitelli [:Dexter]]

(In reply to Ben Wu [:benwu] from comment #40)

We've been receiving telemetry for this in the data pipeline for a few weeks now but it's been coming with the firefox-desktop app namespace

Hey Chris, aren't we changing the app id somewhere for this?

Comment 43

[Alessio Placitelli [:Dexter]]

(In reply to Alessandro Castellani [:aleca] from comment #41)

Is following the steps to add the probe-scraper all we need?

No, we need legal clearance for that step

This is a different problem I believe: thunderbird should be using a different app id

Comment 44

[Chris H-C :chutten]

The only place where that app id is set is in BrowserGlue which I was under the understanding that Thunderbird didn't actually run. If Thunderbird does run those idle tasks, then we're going to need an if (<is thunderbird>) { return; } somewhere around here. (( Actually, looks like those tasks have condition support, so maybe we should prefer condition: <isn't thunderbird> ))

Comment 45

[Martin Giger [:freaktechnik]]

We seem to call that from our own glue: https://searchfox.org/comm-central/rev/d9f4b21312781d3abb9c88cade1d077b9e1622f4/mail/components/MailGlue.sys.mjs#837

So I assume we'd have to pass in an app ID override there (based on https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/toolkit/components/glean/xpcom/nsIFOG.idl#23)?

Comment 46

[Chris H-C :chutten]

That's what's waiting on Legal before we do it. For now, if there's something e.g. in AppConstants we could check as a task condition, that could be ideal

Comment 47

[Alessio Placitelli [:Dexter]]

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

That's what's waiting on Legal before we do it. For now, if there's something e.g. in AppConstants we could check as a task condition, that could be ideal

Adding a different app id, without enabling it into the pipeline, it's fine too I believe

Comment 48

[Magnus Melin [:mkmelin]]

Still waiting on legal and such, so we can properly verify stuff, so no uplifts yet.

Comment 49

[Alessio Placitelli [:Dexter]]

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

Still waiting on legal and such, so we can properly verify stuff, so no uplifts yet.

We got the go-ahead from legal - regardless, we still need to uplift, as this is currently reporting using the wrong app id.

Comment 50

[Alessio Placitelli [:Dexter]]

I mean we need to uplift bug 1910837

Comment 51

[Magnus Melin [:mkmelin]]

Thanks, I've requested beta uplift for that one.
(Will get to this one later, but no time right now.)

Comment 52

[Alessio Placitelli [:Dexter]]

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

Thanks, I've requested beta uplift for that one.
(Will get to this one later, but no time right now.)

Thank you! Yes, that's fine, the important one was the other one :-)