Closed Bug 1597180 Opened 5 years ago Closed 4 years ago

[Thunderbird Telemetry] Enable telemetry reporting in appropriate builds.

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr78+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 74.0
Tracking Flags:
Tracking Status
thunderbird_esr78 + fixed

People

(Reporter: benc, Assigned: benc)

References

Details

(Whiteboard: [TM:78.something])

Attachments

(2 files, 3 obsolete files)

1597180-telemetry-config-and-notes-3.patch
10.46 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
1597180-server-url.patch
1.42 KB, patch
mkmelin
: review+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → benc

The bulk of this patch is just notes on getting telemetry flowing. Details in mail/components/telemetry/README.md.

To get telemetry pings reporting locally:

  1. Rebuild with this patch.
  2. in your prefs.js, add most of the settings I recommend under the "Runtime prefs for testing" section in the README.
  3. run a local web server to display the data locally (see links in README)
  4. try a test ping to make sure it's all set up correctly (again, see README)

If you add the html/text composition probe in Bug 1584889, you should be able to see ping data reported with tb.compose.format_html and tb.compose.format_plain_text counts.

The data is reported at shutdown or after a submission interval (which defaults to hours. The README has info on reducing this for testing).

Attachment #9114470 - Flags: feedback?(mkmelin+mozilla)

Oh, my patch sets the endpoint thusly:

pref("toolkit.telemetry.server", "https://incoming.telemetry.thunderbird.net");

Obviously this needs a little coordination at the server end :-)
Sancus: Is this sane or do you have another URL in mind?

Flags: needinfo?(sancus)
Comment on attachment 9114470 [details] [diff] [review]
1597180-telemetry-config-and-notes-1.patch

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

::: mail/app/profile/all-thunderbird.js
@@ +1248,5 @@
>  // Should be removed in https://bugzilla.mozilla.org/show_bug.cgi?id=1596037
>  pref("devtools.whatsnew.feature-enabled", true);
> +
> +
> +

some extra empty lines here ^^

@@ +1252,5 @@
> +
> +// Telemetry settings.
> +
> +// Server to submit telemetry pings to.
> +pref("toolkit.telemetry.server", "https://incoming.telemetry.thunderbird.net");

Maybe we can just set this to http://localhost:12345 until the server side is set up and privacy policy is properly in place. Just to get it landed but not really sending anything anywhere yet. Then testers would have an easy time just setting the pref to something else while testing

@@ +1258,5 @@
> +pref("toolkit.telemetry.server_owner", "Thunderbird");
> +
> +// Determines if Telemetry pings can be archived locally.
> +pref("toolkit.telemetry.archive.enabled", true);
> +// Enables sending the shutdown ping when Firefox shuts down.

Thunderbird

@@ +1266,5 @@
> +// Enables sending a duplicate of the first shutdown ping from the first session.
> +pref("toolkit.telemetry.firstShutdownPing.enabled", true);
> +// Enables sending the 'new-profile' ping on new profiles.
> +pref("toolkit.telemetry.newProfilePing.enabled", true);
> +// Enables sending 'update' pings on Firefox updates.

Thunderbird

@@ +1278,5 @@
> +  pref("toolkit.telemetry.ecosystemtelemetry.enabled", false);
> +#endif
> +pref("toolkit.telemetry.infoURL", "https://www.mozilla.org/thunderbird/legal/privacy/#telemetry");
> +
> +

nit: there's an extra empty line or two here

::: mail/components/telemetry/README.md
@@ +66,4 @@
>  user_pref("toolkit.telemetry.log.level", "Trace");
> +```
> +
> +The output will show up on the javascript console:

Let's just call it the console.

@@ +72,5 @@
> +
> +If pings aren't showing up, keep an eye out for messages in the console.
> +
> +
> +`datareporting.healthreport.uploadEnabled`  - TODO: set in all.js?

The pref gets set when you set MOZ_SERVICES_HEALTHREPORT. But we probably don't want this, at least atm. I think healthreport comes with a bunch of other UI too.

@@ +81,4 @@
>  user_pref("toolkit.telemetry.send.overrideOfficialCheck", true);
>  ```
>  
> +Without this, telemetry is only sent for official builds.

Maybe clearer to write "Without toolkit.telemetry.send.overrideOfficialCheck set, Telemetry........."

@@ +109,2 @@
>  
> +From the javascript console, you can send an immediate test ping:

console. Or should we call it the DevTools console?

@@ +125,5 @@
> +
> +If `toolkit.telemetry.unified` is false, then the intended-to-be-deprecated `toolkit.telemetry.enabled` controls the result.
> +We're using unified telemetry, so this shouldn't be an issue.
> +
> +

nit: double empty line
Attachment #9114470 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED

OK, this version removes the MOZ_SERVICES_HEALTHREPORT compiletime option and refines the notes a bit.

Here are example steps to see some telemetry pings:

  1. Apply Bug 1584889 (to record html/text composition)
  2. Apply this patch
  3. build
  4. to your profile dir, add user.js:
user_pref("toolkit.telemetry.server", "http://localhost:12345");
user_pref("toolkit.telemetry.server_owner", "TimmyTestfish");
user_pref("toolkit.telemetry.log.level", "Trace");
user_pref("toolkit.telemetry.log.dump", true);
user_pref("toolkit.telemetry.send.overrideOfficialCheck", true);
user_pref("datareporting.policy.dataSubmissionPolicyBypassNotification",true);
user_pref("services.sync.telemetry.submissionInterval", 20);
user_pref("datareporting.policy.dataSubmissionEnabled", true);
user_pref("datareporting.healthreport.uploadEnabled",true);
  1. run a local web server (I use webhole -a :12345, but you'll probably find gzipServer simpler unless you've got golang installed).
  2. run Thunderbird
  3. start writing a message, to log the text/html composition scalar. It is recorded as the composition is opened. You can close the window immediately.
  4. Wait 20 sec or so (submissionInterval, above), or close TB to send pending pings.
  5. see the lovely data appear on the server side. The html/text scalars are counted in tb.compose.format_html/tb.compose.format_plain_text.
Attachment #9114470 - Attachment is obsolete: true
Attachment #9115066 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9115066 [details] [diff] [review]
1597180-telemetry-config-and-notes-2.patch

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

::: mail/app/profile/all-thunderbird.js
@@ +1250,5 @@
> +
> +// Telemetry settings.
> +
> +// Server to submit telemetry pings to.
> +//pref("toolkit.telemetry.server", "https://incoming.telemetry.thunderbird.net");

If we don't have that, let's leave it out for now

@@ +1255,5 @@
> +// Telemetry server owner. Please change if you set toolkit.telemetry.server to a different server
> +//pref("toolkit.telemetry.server_owner", "Thunderbird");
> +// But for now we'll just default to local testing:
> +pref("toolkit.telemetry.server", "http://localhost:8080");
> +pref("toolkit.telemetry.server_owner", "LocalTestServerOwner");

for this one it should be ok to just use Thunderbird from start

@@ +1278,5 @@
> +#else
> +  pref("toolkit.telemetry.ecosystemtelemetry.enabled", false);
> +#endif
> +
> +// Required to enable telemetry pings (defaults true if

defaults to true if
Attachment #9115066 - Flags: feedback?(mkmelin+mozilla) → feedback+

magnus: new version with the tweaks you suggested. If you're still happy with it we can land it.

Attachment #9115066 - Attachment is obsolete: true
Attachment #9120379 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9120379 [details] [diff] [review]
1597180-telemetry-config-and-notes-3.patch

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

Seems alright, I've sent it for a quick try run now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a5b8315d43217755e7f367fa4bcfea5377a34cb2

::: mail/components/telemetry/README.md
@@ +128,4 @@
>  
>  ```
>  Cu.import("resource://gre/modules/TelemetrySession.jsm");
>  TelemetrySession.testPing()

nit: missing ;
Attachment #9120379 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f8a6cf1ee08f
Add build and prefs config to enable Thunderbird telemetry reporting. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 74.0
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1784150a888a
followup to fix inproper quotes in all-thunderbird.js. r=me DONTBUILD

One of my profiles didn't want to start up with the backticks for quotes. Not sure it's really supported in that file. No idea why it's not consistent about failing though, but I pushed the fix.

l10n items are all complete except the privacy policy? (thinking of the string freeze so just double checking)

Flags: needinfo?(mkmelin+mozilla)

I filed bug 1640897 to update the UI.

Flags: needinfo?(mkmelin+mozilla)

I'm getting an error on current Nightly when trying to get the client to send reports. It works correctly in Firefox(and fills up the server with reports, so that part's working).

Cu.import("resource://gre/modules/TelemetrySession.jsm");
TelemetrySession.testPing()
TypeError: can't access property "getFullYear", date is undefined TelemetryUtils.jsm:167:1

Other than this error, https://incoming-telemetry.thunderbird.net is working so assuming this can be resolved(or is somehow a configuration issue on my side) we can change the pref and start taking reports from Nightly.

Flags: needinfo?(sancus)

Ping, can you take a look at ^^^ and see if you can reproduce?

I just tried that in the Error Console, and didn't get any error.

Flags: needinfo?(remotenonsense)

I don't see any error in my console as well. Maybe related to some configs.

Flags: needinfo?(remotenonsense)

There are some early returns above this line: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/app/TelemetryController.jsm#845, I think session timestamps were initiated in this earlyInit function.

Note that this is on Windows. I just tried a completely fresh profile and still got that error.

I will try setting up the dev environment on Windows and get back to you later.

I did a fresh build on Windows, still no error. Then I downloaded the latest nightly installer from ftp.mozilla.org and can see the error.

(In reply to Ping Chen (:rnons) from comment #19)

I did a fresh build on Windows, still no error. Then I downloaded the latest nightly installer from ftp.mozilla.org and can see the error.

There are a bunch of settings documented in the Telemetry README https://searchfox.org/comm-central/source/mail/components/telemetry/README.md#108

I suspect that you need to set these before a fresh build will properly try to send a Telemetry ping, so that may be why you're not seeing it. I do still see it on Nightly(even after update to 2020-06-08), so, unless it is some weird Nightly build configuration issue, it's probably still bugged on trunk.

I think the configs you linked are to help investigating telemetry report data on local, basically sending the report to a local server. They're not required to run these two lines

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

I have no clue why the Nightly build has this error. Is it possible to reproduce the nightly build on local?

To get the same build config as a nightly build, check https://searchfox.org/comm-central/source/mozilla/browser/config/mozconfigs/linux64/nightly

checking about:buildconfig could also be useful (available through Help | Troubleshooting information)

I did a build on local with MOZ_AUTOMATION=1 MOZ_FETCHES_DIR=/home/rnons/.mozbuild MOZCONFIG=comm/mail/config/mozconfigs/linux64/nightly ./mach build, I would expect that to be mostly equivalent to the nightly build. However, still no error.

I don't find much info on about:buildconfig page, Configure options are mostly equivalent to the nightly build

MOZ_AUTOMATION=1 --enable-application=comm/mail MOZILLA_OFFICIAL=1 --enable-update-channel= CC=/home/rnons/.mozbuild/clang/bin/clang CXX=/home/rnons/.mozbuild/clang/bin/clang++ NASM=/home/rnons/.mozbuild/nasm/nasm ENABLE_CLANG_PLUGIN=1 MOZ_STDCXX_COMPAT=1 MOZ_NO_PIE_COMPAT=1 RUSTC=/home/rnons/.cargo/bin/rustc CARGO=/home/rnons/.cargo/bin/cargo RUSTDOC=/home/rnons/.cargo/bin/rustdoc CBINDGEN=/home/rnons/.cargo/bin/cbindgen RUSTFMT=/home/rnons/.cargo/bin/rustfmt --enable-js-shell --enable-rust-simd NODEJS=/home/rnons/.mozbuild/node/bin/node --enable-default-toolkit=cairo-gtk3-wayland MAKE=/usr/bin/make --enable-crashreporter

Configure options of the Nightly build are

MOZ_AUTOMATION=1 --enable-application=comm/mail MOZILLA_OFFICIAL=1 --enable-update-channel=nightly MOZBUILD_STATE_PATH=/builds/worker/.mozbuild CC=/builds/worker/fetches/clang/bin/clang CXX=/builds/worker/fetches/clang/bin/clang++ NASM=/builds/worker/fetches/nasm/nasm ENABLE_CLANG_PLUGIN=1 MOZ_STDCXX_COMPAT=1 MOZ_NO_PIE_COMPAT=1 RUSTC=/builds/worker/fetches/rustc/bin/rustc CARGO=/builds/worker/fetches/rustc/bin/cargo RUSTDOC=/builds/worker/fetches/rustc/bin/rustdoc CBINDGEN=/builds/worker/fetches/cbindgen/cbindgen RUSTFMT=/builds/worker/fetches/rustc/bin/rustfmt --enable-js-shell --enable-rust-simd NODEJS=/builds/worker/fetches/node/bin/node --enable-default-toolkit=cairo-gtk3-wayland MAKE=/usr/bin/make --enable-crashreporter

Testing on 78.0b1 build4 still produces the error, I was hoping it was something to do with Nightly's build config specifically, but it seems like not.

Attached patch 1597180-package-telemetry.patch (obsolete) — — Splinter Review

This patch added TelemetryStartup.manifest to package-manifest.in. See the same line in m-c https://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in#191.

TelemetryStartup is needed to forward the "profile-after-change" notification to TelemetryController.jsm.
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryStartup.jsm#19-23

I can't reproduce the error in comment 23, because I was running ./dist/bin/thunderbird and dist/bin/components/TelemetryStartup.manifest exists. The problem is TelemetryStartup.manifest is not included in the released package.

Attachment #9157178 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9157178 [details] [diff] [review]
1597180-package-telemetry.patch

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

Makes sense, thx!! r=mkmelin
Attachment #9157178 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9157178 [details] [diff] [review]
1597180-package-telemetry.patch

Needed for beta so that telemetry would send data for packaged (=normal) builds.
Attachment #9157178 - Flags: approval-comm-beta?

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d8c73b98530c
Include TelemetryStartup in package manifest so that telemetry reporting works in packaged builds. r=mkmelin

Keywords: checkin-needed-tb

Looks like this patch is causing lots of telemetry test failures. Well, that was unexpected! E.g. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306623142&repo=comm-central&lineNumber=2254
I've backed it out for now: https://hg.mozilla.org/comm-central/rev/1266261951c7599b81738f7855968a0edda9e91a

Attachment #9157178 - Flags: approval-comm-beta?

I can't figure out why this one line change breaks tests. Seems all failing tests are keyed scalar probes, normal scalar and histogram tests passed. But I don't know how to explain that.

A Try run link: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aaa7b53b8ecc74f69d5c33e8575cbb4f51c9793a

Possibly they need release_channel_collection: opt-out in the definition.
I tried that with one - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f76b207996b7f4a9a706e78bd1853aa923caefa2&selectedTaskRun=PW9x-_LiT-eUuDwY9ruxDA.0 - and that doesn't show a problem for the probe I changed (different from the one you have in comment 30)

Sent off another try run with more (all) of them changed now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=91097a54deb8f43fb3889ef9af017a5453ee6b14

That seems to have done the trick, but bug 1646724 is still there.

I'll go ahead and land that change so we can start getting telemetry data. If bug 1646724 is persistent we need to check that and possibly disable it.

Awesome, I didn't notice release_channel_collection at all. Will see if I can find anything about bug 1646724.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3bff18148f7b
Include TelemetryStartup in package manifest so that telemetry reporting works. r=mkmelin
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/850f82431c23
fix test failures of comm/mail/test/browser/cloudfile/browser_filelinkTelemetry.js: tb.filelink.ignored. rs=me

Update server url according to comment 13.

Attachment #9158097 - Flags: review?(mkmelin+mozilla)
Attachment #9158097 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c208d61f7cc6
Update telemetry server pref to production url. r=mkmelin

Keywords: checkin-needed-tb

Flags so we don't forget to uplift.
Also, do we need 1597180-package-telemetry.patch on esr78 ?

status-thunderbird_esr78: --- → affected
tracking-thunderbird_esr78: --- → +
Flags: needinfo?(remotenonsense)
Comment on attachment 9158097 [details] [diff] [review]
1597180-server-url.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: no telemetry
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): no risk, just a URL change
Attachment #9158097 - Flags: approval-comm-esr78?
Attachment #9157178 - Attachment is obsolete: true
Flags: needinfo?(remotenonsense)

(In reply to Wayne Mery (:wsmwk) from comment #40)

Also, do we need 1597180-package-telemetry.patch on esr78 ?

I marked it as obsolete just now, it was replaced by the commits in comment 35, 37. I just checked, they have already been pushed to comm-beta.
https://hg.mozilla.org/releases/comm-beta/rev/3bff18148f7b67b4f6d14773697a47ac4d12ab16

(In reply to Wayne Mery (:wsmwk) from comment #41)

1597180-server-url.patch

This is in comm-beta as well.
https://hg.mozilla.org/releases/comm-beta/rev/c208d61f7cc688d2ad79e864d77e91392b8d0b8d

Whiteboard: [TM:78.1.0]
See Also: → 1651298

Ryan, let me know when the website is up.

Flags: needinfo?(ryan)
Whiteboard: [TM:78.1.0] → [TM:78.something]

It should go up here: https://www.mozilla.org/privacy/thunderbird/

Hopefully as early as today, but it is still in a final review. I will update this bug as soon as it is live.

Flags: needinfo?(ryan)

I don't think it's going to be up today, there's still some discussion about some bits related to Gandi and the use of the term "Mozilla".

Comment on attachment 9158097 [details] [diff] [review]
1597180-server-url.patch

[Triage Comment]
Approved for esr78

Attachment #9158097 - Flags: approval-comm-esr78? → approval-comm-esr78+

The changes mentioned in comments 35 and 37 would have gone into 79beta, and are not present on 78esr. Do those need to be taken as well? Then if that's the case there is a merge conflict because beta has diverged quite a bit from what's on esr78 somehow.

We need all the changesets yes. But I would wait and add this to 78.3 instead, along with bug 1651298.

Is the intention to have telemetry on 78 collect everything that's on beta right now? The Scalars.yaml files have diverged enough that my last uplift attempt ran into some merge issues with that file. It looks like we need to uplift bug 1641773 (79) and bug 1644311 (79) in addition to this one in order to fix that. (requested)
Plus these from this bug (for uplifter reference):
https://hg.mozilla.org/comm-central/rev/3bff18148f7b
https://hg.mozilla.org/comm-central/rev/850f82431c23
https://hg.mozilla.org/comm-central/rev/c208d61f7cc6

Yes, we want all the probes we've created.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: