1615996 - [Thunderbird Telemetry] collect type of composition: new/reply/fwd/edit draft etc.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Magnus Melin [:mkmelin]]

To understand the typical usage pattern of our users, it would be interesting to collect what composition type is: new/reply/forward/draft etc.

Comment 1

[Magnus Melin [:mkmelin]]

See bug 1584889.

Comment 2

[Ping Chen [:rnons] (away)]

Attachment: 1615996.patch

This patch

1. defined TB_COMPOSE_TYPE as categorical type in Histograms.json
2. use Telemetry::Accumulate in ngMsgCompose.cpp

The report I get from gzipServer seems alright

     "TB_COMPOSE_TYPE": {
        "bucket_count": 51,
        "histogram_type": 5,
        "sum": 12,
        "range": [
          1,
          50
        ],
        "values": {
          "5": 0,
          "6": 2,
          "7": 0
        }
      }

But the test is failing with

Unexpected Results
------------------
comm/mailnews/compose/test/unit/test_telemetry_compose.js
  FAIL test_compose_type - [test_compose_type : 304] Should have found an entry for TB_COMPOSE_TYPE at index 0 - false == true
resource://testing-common/TelemetryTestUtils.jsm:assertHistogram:304

I tried to print out Services.telemetry.getHistogramById('TB_COMPOSE_TYPE'), but get an empty object.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9150059 [details] [diff] [review]
1615996.patch

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

::: mailnews/compose/test/unit/test_telemetry_compose.js
@@ +90,5 @@
> +    createCompose(Ci.nsIMsgCompType.ReplyIgnoreQuote);
> +  }
> +
> +  // Did we count them correctly?
> +  const histogram = TelemetryTestUtils.getAndClearHistogram("TB_COMPOSE_TYPE");

note the name of that: get AND clear. So you'll always get an empty object.

I think you want to get it before you do all the calls, then, looks like histogram.snapshot() will be what you want?

Comment 4

[Ping Chen [:rnons] (away)]

Attachment: 1615996.patch

Yes, you're right, I moved getAndClearHistogram above. Also found assertHistogram is not what I want, it tries to test other buckets are empty, not sure why is it designed that way.
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/utils/TelemetryTestUtils.jsm#296-301

Tests pass now, thank you.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9150073 [details] [diff] [review]
1615996.patch

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

Looks good, thx! r=mkmelin
I've sent it off to the try server for you now to make sure: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4e8bc41b751c87f0a5d72029213e7f483615e80f

Comment 6

[Magnus Melin [:mkmelin]]

That looked good, just known oranges.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9150073 [details] [diff] [review]
1615996.patch

This isn't right. ReplyIgnoreQuote is not a compose type. Please check its definition and use.

Comment 8

[Ping Chen [:rnons] (away)]

Attachment: 1615996.patch

(In reply to Jorg K (CEST = GMT+2) from comment #7)

Comment on attachment 9150073 [details] [diff] [review]
1615996.patch

This isn't right. ReplyIgnoreQuote is not a compose type. Please check its
definition and use.

Thanks, didn't know about that. Please see if it's better now.

Comment 9

[Magnus Melin [:mkmelin]]

Ugh, that's just so wrong. I filed bug 1639452. In this bug I think all that needs to be done is removing that type.

Comment 10

[Ping Chen [:rnons] (away)]

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

Ugh, that's just so wrong. I filed bug 1639452. In this bug I think all that needs to be done is removing that type.

Hi, just making sure, do you mean all I need to do now is removing this test?

  Assert.equal(
    snapshot.values[Ci.nsIMsgCompType.ReplyIgnoreQuote],
    undefined,
    "nsIMsgCompType.ForwardInline count must be correct"
  );

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9150336 [details] [diff] [review]
1615996.patch

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

::: mailnews/compose/test/unit/test_telemetry_compose.js
@@ +105,5 @@
> +  );
> +  Assert.equal(
> +    snapshot.values[Ci.nsIMsgCompType.EditTemplate],
> +    NUM_EDIT_TEMPLATE,
> +    "nsIMsgCompType.ReplyIgnoreQuote count must be correct"

yes, and this should be EditTemplate

Comment 12

[Ping Chen [:rnons] (away)]

Attachment: 1615996.patch

Fixed review comments.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9150364 [details] [diff] [review]
1615996.patch

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

Looks good, thx.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9150364 [details] [diff] [review]
1615996.patch

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

Looks OK now, r=jorgk (not jorg). I checked that ReplyIgnoreQuote is pretty short-lived, it gets removed here
https://searchfox.org/comm-central/rev/cfa832291ae06d6b814f8a9ebe1991a08f25da92/mailnews/compose/src/nsMsgComposeService.cpp#357
and doesn't get to where you're reporting the compose type.

Comment 15

[Ping Chen [:rnons] (away)]

Attachment: 1615996.patch

r=jorgk in commit message.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ba49bdd5e52c
Add telemetry to count use of message compose type. r=mkmelin,jorgk DONTBUILD

Comment 17

[Geoff Lankow (:darktrojan)]

The debug test runs did not like this:

TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_telemetry_compose.js | xpcshell return code: -11
PROCESS-CRASH | comm/mailnews/compose/test/unit/test_telemetry_compose.js | application crashed [@ nsMsgCompose::CreateMessage(char const*, int, nsIMsgCompFields*)]

Example log: https://treeherder.mozilla.org/logviewer.html#?job_id=303311380&repo=comm-central

Comment 18

[Magnus Melin [:mkmelin]]

[18975, Main Thread] ###!!! ASSERTION: CreateMessage can't get draft id: 'NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty()', file /builds/worker/checkouts/gecko/comm/mailnews/compose/src/nsMsgCompose.cpp, line 1752

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: bug1615996_draft_assert.patch

I don't think this needs to assert.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9150969 [details] [diff] [review]
bug1615996_draft_assert.patch

You should just set a draft ID in your test instead of modifying the backend. Why is it a good idea to to change established code just because a new test is incomplete? And why don't you run those tests locally in a debug build before landing them?

Comment 21

[Magnus Melin [:mkmelin]]

Since all the code around where I'm changing it already has code comments that make it clear this is an expected - but perhaps not common case - asserting about it makes no sense. Just adding a fake id would just produce further warnings in debug builds - so that does little to improve matters.

Comment 22

[Jorg K (CEST = GMT+2)]

Just adding a fake id would just produce further warnings in debug builds

Which? Have you tried?

        let composeFields = Cc[
          "@mozilla.org/messengercompose/composefields;1"
        ].createInstance(Ci.nsIMsgCompFields);
        composeFields.draftId = "XXX";
        params.composeFields = composeFields;

is all you need.

EDIT: Makes the test more life-like. You could also set params.format = Ci.nsIMsgCompFormat.Default;.

Comment 23

[Magnus Melin [:mkmelin]]

All that would do is get you into the next assertion a few lines down "CreateMessage can't get msg header DB interface pointer." - which is kind of a valid assertion given we did go down that if-clause.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9150969 [details] [diff] [review]
bug1615996_draft_assert.patch

OK then ;-)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a077945683b
don't assert for missing draftid - warn only. r=jorgk DONTBUILD