1615985 - [Thunderbird Telemetry] collect number of calendars + types

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin]]

It would be useful for product design to understand how many calendars people typically have set up, and what type those calendars are. Readonly status would also be of interest.

Comment 1

[Ping Chen [:rnons] (away)]

Attachment: 1615985.patch

Report calendar count and read only calendar count in loadCalendarManager.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9153352 [details] [diff] [review]
1615985.patch

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

LGTM, but let's have Paul review this (it's in calendar/)

Comment 3

[Paul Morris [:pmorris]]

Comment on attachment 9153352 [details] [diff] [review]
1615985.patch

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

Looks good overall.  Will be great to have this information about calendar usage.  I just have a few questions and minor requests.

::: calendar/base/content/calendar-management.js
@@ +142,5 @@
> +    Services.telemetry.keyedScalarSet(
> +      "tb.calendar.read_only_count",
> +      type.toLowerCase(),
> +      readOnlyCount
> +    );

So this telemetry code runs even if the user has opted out of telemetry?  I'm just curious about how it works, and whether this should only run if a telemetry pref is set or something similar.

::: calendar/test/browser/browser_calendarTelemetry.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

We usually use Mozilla Public License, so I'd want to confirm that Public Domain is acceptable.  I guess that's a question for Magnus.

@@ +26,5 @@
> +  }
> +
> +  await loadCalendarManager();
> +  let scalars = TelemetryTestUtils.getProcessScalars("parent", true);
> +  Assert.equal(scalars["tb.calendar.count"]["memory"], 3, "Count of calendars must be correct.");

Just a nit: we would typically phrase this like "Count of calendars is correct." so better to go with that for consistency, here and below.

::: mail/components/telemetry/Scalars.yaml
@@ +147,5 @@
>      record_in_processes:
>        - 'main'
> +
> +tb.calendar:
> +  count:

"calendar" is often used as a namespace for the calendar component, so to do that here as well, I think it would be clearer to use: `tb.calendar.calendar_count` and `tb.calendar.read_only_calendar_count`.  Then in the future if we want to count other things related to calendars the names will still be clear.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #3)

So this telemetry code runs even if the user has opted out of telemetry?

The calls just add the collection. Enabled/disabled is just handled later on. If disabled the data will just be ignored.

We usually use Mozilla Public License, so I'd want to confirm that Public
Domain is acceptable. I guess that's a question for Magnus.

Some tests are public domain. But I guess we could use MPL.

Just a nit: we would typically phrase this like "Count of calendars is
correct." so better to go with that for consistency, here and below.

The problem with this is, then when you get an assertion it says "Count of calendars is correct." - but it's wrong!

Comment 5

[Paul Morris [:pmorris]]

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

The calls just add the collection. Enabled/disabled is just handled later on. If disabled the data will just be ignored.

I thought that was likely the case. (It would be nicer if the code wouldn't run at all when the data wasn't going to be reported, but oh well...)

Some tests are public domain. But I guess we could use MPL.

Okay, just wanted to check what our stance was on that. I don't have a strong opinion either way.

The problem with this is, then when you get an assertion it says "Count of calendars is correct." - but it's wrong!

Good point! We should probably use "must be correct" and similar phrasing, as that would be clearer when you get a failure.

Comment 6

[Ping Chen [:rnons] (away)]

Attachment: 1615985.patch

Updated scalar key names as suggested.

About the license, I just copied it from other test files. After your comment, I did some search and found https://bugzilla.mozilla.org/show_bug.cgi?id=1073556 and https://www.mozilla.org/en-US/MPL/license-policy/. Seems it's recommend to use public domain for test files.

Trivial bits of Mozilla Code, such as testcases or snippets of code used in documentation, should be put in the public domain in order to facilitate re-use.

Comment 7

[Paul Morris [:pmorris]]

Comment on attachment 9153700 [details] [diff] [review]
1615985.patch

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

Looks good, thanks for the changes.  r+ but Geoff mentioned on chat that the test fails on Windows and Mac, so we should resolve that before checking this in.  Also, good to know about public domain license usage.

Comment 8

[Ping Chen [:rnons] (away)]

Thanks. The failing test is from bug 1615981, I'm still investigating.

Comment 9

[Paul Morris [:pmorris]]

Okay, since the test failure is related to another bug, then this should be ready to land. Usually we put a link to a successful try run here in the bug.

Comment 10

[Ping Chen [:rnons] (away)]

This is a try run https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f0bb17850de33fa8c18062f73ee179617cec6a2. Not quite sure, but seems the oranges are not related to this patch.

Comment 11

[Ping Chen [:rnons] (away)]

I found the failing tests in the Try run are all from comm/calendar/test/browser/, so maybe related to this patch. Then I tried on my local, but even without applying this patch, ./mach test comm/calendar/test/browser/browser_calendarList.js failed with time out.

 ...
 0:12.58 INFO New window opened: chrome://calendar/content/calendar-properties-dialog.xhtml
 0:12.58 PASS undefined assertion name -
 0:12.58 PASS undefined assertion name -
 0:12.90 PASS undefined assertion name -
 0:12.90 PASS undefined assertion name -
 0:12.90 PASS undefined assertion name -
 0:12.90 PASS undefined assertion name -
 0:56.40 INFO Failed to retrieve MOZ_UPLOAD_DIR env var
 0:56.39 FAIL Test timed out -
 0:56.42 PASS calendar tab is not open -
 0:56.51 PASS tasks tab is not open -
 ...
 0:56.54 TEST_END: Test OK. Subtests passed 90/91. Unexpected 1

Will investigate tomorrow.

Comment 12

[Paul Morris [:pmorris]]

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

This is a try run https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f0bb17850de33fa8c18062f73ee179617cec6a2. Not quite sure, but seems the oranges are not related to this patch.

I just took a look, and those oranges do appear to be unrelated to this patch. So I'd say this is ready to check in.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6602b6ca210f
Collect number of calendars with types and readonly status. r=mkmelin,pmorris

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d287cf040cba
followup to lint 6602b6ca210fe75a9e65d19acc80635b287a52fe. rs=eslint DONTBUILD

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e2b05188141
Backed out two changesets for breaking calendar tests. rs=backout

Comment 16

[Geoff Lankow (:darktrojan)]

Sorry, but I've backed this out. It was causing the failures you saw on your Try run. (Do Try runs on two or more platforms so you can be more certain whether failures are your fault or just noise.)

Also I'm not happy with the patch itself. This telemetry should be in the back-end CalCalendarManager.jsm (and probably take into account the addition/removal of calendars once the program's running – not sure how that sort of thing is meant to be handled in telemetry) instead of in the front-end code. As it is the code runs for every Thunderbird main window that opens (probably not desired) and before your test runs, which is why you're having to call loadCalendarManager in the test.

Comment 17

[Ping Chen [:rnons] (away)]

Thanks, I will take care.

probably take into account the addition/removal of calendars once the program's running

Maybe I'm wrong, but I think telemetry reports don't need to reflect the real-time status, they give us general idea of how users are using TB. Perhaps it's good enough that the reports are updated when TB is restarted next time. I can change it if you think that's better.

which is why you're having to call loadCalendarManager in the test.

I didn't find a way to inject mock data (calendars) before the mochitest browser starts. I was calling loadCalendarManager to run the telemetry probe. I will split it out to a reportCalendars function and call it directly in the test.

Comment 18

[Magnus Melin [:mkmelin]]

Correct, Telemetry is not real-time status. It just accumulates and usually sent off like once a day, so I don't think we need to worry about added/removed calendars, which will be rare. Chances are that intermittent state would never make it to the server anyway.

Comment 19

[Ping Chen [:rnons] (away)]

Attachment: 1615985.patch

• Split out reportCalendars function to be called directly in test

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

Comment 20

[Geoff Lankow (:darktrojan)]

Yeah, you're right, I was thinking about what the test was trying to do at the time.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9154852 [details] [diff] [review]
1615985.patch

Yeah, okay. That is better.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5537fe1e0145
Collect number of calendars with types and readonly status. r=mkmelin,pmorris,darktrojan DONTBUILD

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9154852 [details] [diff] [review]
1615985.patch

Telemetry addition - not risky.

Comment 24

[Geoff Lankow (:darktrojan)]

Thunderbird 78.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/3d89c61c6302

Comment 25

[Geoff Lankow (:darktrojan)]

Correction to the previous uplift which Hg somehow messed up:
https://hg.mozilla.org/releases/comm-beta/rev/d2d84ac207536f6f672b861964909d1c70d46da0