Closed Bug 1615984 Opened 7 months ago Closed 4 months ago

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

Categories

(Thunderbird :: FileLink, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: mkmelin, Assigned: rnons)

Details

Attachments

(1 file, 3 obsolete files)

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.

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

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

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

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

Attached patch 1615984.patch (obsolete) — Splinter Review

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.

Attachment #9151940 - Attachment is obsolete: true
Attachment #9152315 - Flags: review?(mkmelin+mozilla)
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
Attachment #9152315 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 1615984.patch (obsolete) — Splinter Review

Fixed now. Thx

Attachment #9152315 - Attachment is obsolete: true
Attachment #9152324 - Flags: review?(mkmelin+mozilla)
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"
Attachment #9152324 - Flags: review?(mkmelin+mozilla)
Attached patch 1615984.patchSplinter Review

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

Attachment #9152324 - Attachment is obsolete: true
Attachment #9152385 - Flags: review?(mkmelin+mozilla)
Attachment #9152385 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 78.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/26f9f28bdccc
Collect filelink usage: total file uploaded and ignored times. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.