Closed Bug 1615985 Opened 2 years ago Closed 1 year ago

[Thunderbird Telemetry] collect number of calendars + types

Categories

(Calendar :: General, task)

task
Not set
normal

Tracking

(thunderbird78 fixed)

RESOLVED FIXED
Thunderbird 78.0
Tracking Status
thunderbird78 --- fixed

People

(Reporter: mkmelin, Assigned: rnons)

Details

Attachments

(1 file, 2 obsolete files)

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.

Assignee: nobody → remotenonsense
Attached patch 1615985.patch (obsolete) — Splinter Review

Report calendar count and read only calendar count in loadCalendarManager.

Attachment #9153352 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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/)
Attachment #9153352 - Flags: review?(mkmelin+mozilla) → review?(paul)
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.
Attachment #9153352 - Flags: review?(paul) → feedback+

(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!

(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.

Attached patch 1615985.patch (obsolete) — Splinter Review

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.

Attachment #9153352 - Attachment is obsolete: true
Attachment #9153700 - Flags: review?(paul)
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.
Attachment #9153700 - Flags: review?(paul) → review+

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

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.

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 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.

(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.

Target Milestone: --- → 79

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d287cf040cba
followup to lint 6602b6ca210fe75a9e65d19acc80635b287a52fe. rs=eslint DONTBUILD
Backout by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1e2b05188141
Backed out two changesets for breaking calendar tests. rs=backout

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.

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.

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.

Attached patch 1615985.patchSplinter Review
  • Split out reportCalendars function to be called directly in test

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

Attachment #9153700 - Attachment is obsolete: true
Attachment #9154852 - Flags: review?(geoff)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

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

Yeah, okay. That is better.
Attachment #9154852 - Flags: review?(geoff) → review+

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

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Comment on attachment 9154852 [details] [diff] [review]
1615985.patch

Telemetry addition - not risky.
Attachment #9154852 - Flags: approval-calendar-beta?(philipp)
Attachment #9154852 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Target Milestone: 79 → 78
You need to log in before you can comment on or make changes to this bug.