1615984 - [Thunderbird Telemetry] collect filelink usage: size sent total, times not used even though size was above threshold

Status: RESOLVED FIXED

(Thunderbird :: FileLink, task)

Description

[Magnus Melin [:mkmelin]]

Would be useful to know how much the filelink feature is used. Let's collect the typical sizes sent, and for which sizes people don't want to use it.

Comment 1

[Ping Chen [:rnons] (away)]

Attachment: 1615984.patch

This patch added

1. a probe to collect total file size in bytes uploaded to filelink providers
2. a probe to collect how many times filelink suggestion are ignored

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9151940 [details] [diff] [review]
1615984.patch

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

I think it would be good to add one more thing: the display name of the provider used: cloudFileAccounts.getDisplayName(cloudFileAccount).
That may require changing the data structure a bit.

::: mail/components/telemetry/Scalars.yaml
@@ +94,5 @@
> +
> +  ignored:
> +    bug_numbers:
> +      - 1615984
> +    description: How many times filelink suggestion are ignored.

I think what this patch is doing is counting the active ignores, the dismissals.
We could add those, but I would like to know the passive ignores too: the case where you send with large files, but did not use filelink even if suggested (just left the notification bar alone).

Comment 3

[Ping Chen [:rnons] (away)]

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

I think it would be good to add one more thing: the display name of the
provider used: cloudFileAccounts.getDisplayName(cloudFileAccount).
That may require changing the data structure a bit.

Seems to me both keyed scalar and histogram require pre-defined key/label list, so we can only collect wetransfer and existing filelink extensions providers. Is that alright? I will probably use histogram probe.

Comment 4

[Magnus Melin [:mkmelin]]

Or should it be events? https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/collection/events.html#recordevent
This could record the event with provider name + size

Comment 5

[Ping Chen [:rnons] (away)]

Attachment: 1615984.patch

Found out that for keyed scalar, even when keyed: true, keys field is still optional. So I think it's suitable here.

Also changed tb.filelink.ignored to mean big attachment is sent without using filelink.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9152315 [details] [diff] [review]
1615984.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4502,5 @@
> +    msgType == Ci.nsIMsgCompDeliverMode.Now ||
> +    msgType == Ci.nsIMsgCompDeliverMode.Later ||
> +    msgType == Ci.nsIMsgCompDeliverMode.Background
> +  ) {
> +    let kOfferThreshold = "mail.compose.big_attachments.threshold_kb";

since this is used just once, please just inline it

@@ +4505,5 @@
> +  ) {
> +    let kOfferThreshold = "mail.compose.big_attachments.threshold_kb";
> +    let maxSize = Services.prefs.getIntPref(kOfferThreshold, 0) * 1024;
> +    let bucket = document.getElementById("attachmentBucket");
> +    const listItems = [...bucket.itemChildren];

I think too many tmp variables hurt readability - one has to think about if they will be used later or not. So I would suggest 

let items = [...document.getElementById("attachmentBucket").itemChildren];
if (items.some(item => item.attachment.size >= maxSize && !item.attachment.sendViaCloud)) {
  Services.telemetry.scalarAdd("tb.filelink.ignored", 1);
}

::: mail/test/browser/cloudfile/browser_filelinkTelemetry.js
@@ +63,5 @@
> +
> +  Services.prefs.setBoolPref(
> +    kInsertNotificationPref,
> +    oldInsertNotificationPref
> +  );

instead of keeping track of and putting back the old, I think we can just call Services.prefs.clearUserPref to reset

Comment 7

[Ping Chen [:rnons] (away)]

Attachment: 1615984.patch

Fixed now. Thx

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9152324 [details] [diff] [review]
1615984.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4505,5 @@
> +  ) {
> +    let maxSize =
> +      Services.prefs.getIntPref(
> +        "mail.compose.big_attachments.threshold_kb",
> +        0

let's skip the default 0 (don't use a second argument)
If we don't have the pref set something is wrong and we'd rather find that out through and error in the console

::: mail/test/browser/shared-modules/CloudfileHelpers.jsm
@@ +23,5 @@
>  var kDefaults = {
>    type: "default",
>    iconURL: "chrome://messenger/content/extension.svg",
>    accountKey: null,
> +  displayName: "default",

better to use something searchable, like "MockProvider"

Comment 9

[Ping Chen [:rnons] (away)]

Attachment: 1615984.patch

Fixed. As account name can be easily modified, changed to use cloudFileAccount.type as key of tb.filelink.uploaded_size.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26f9f28bdccc
Collect filelink usage: total file uploaded and ignored times. r=mkmelin