681873 - Add basic Telemetry support to Thunderbird

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Jonathan Protzenko [:protz]]

Attachment: Toolkit patch v1

This patch adds three news fields to Telemetry.h, fixes a couple comments, and makes the error code more meaningful in one case.
- Indexing rate: when performing an initial indexing of the messages, how many messages per second we're able to index, in conformance with our cpu hogging ratio.
- Gloda database size: one probe on startup, that tells how large the database is.
- Time to 2nd gloda query for conversations: conversations is an officially supported addon for Thunderbird, and a lot of time is spent querying Gloda, and we'd like to figure out how long people wait on average. There's two subsequent queries, and this aims at measuring how long the two take to complete (there's still templating and rendering time, but these are more easily actionable without getting large field data).

These are the most urgent things to measure, as these are likely to improve significantly once my small queue of patches to Gloda lands.

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: Thunderbird patch v1

Comment 2

[Jonathan Protzenko [:protz]]

Squib, I'm CCing you on the bug so that you can see how easy it actually is to add Telemetry probes. What we could do for a start is:
- count how many times per hour people use detach, or delete,
- probe the size of their screens,
- and much more.

However, the probes above raise bigger privacy concerns than mere sqlite timings; I'm pretty sure they will warrant a privacy review. I'm unsure about the three I'm adding here, so I thought we could spin off a separate bug to add more Thunderbird-specific Telemetry probes.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 555657 [details] [diff] [review]
Toolkit patch v1

I suspect Taras needs to review this.  I may have misled you when I said I reviewed some telemetry probes for storage, I reviewed those for the storage module.

Also, I'm worried my suggestion to #ifdef these might also be wrong, but I don't know how the server works enough to know what is right.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 555658 [details] [diff] [review]
Thunderbird patch v1

This looks fine, although there may be some ambiguity between whether we are just indexing messages really slowly or are working some other type of jobs.  It might be worth adding another flag that is "_messageIndexingJobActive" that gets set when we create a worker for message indexing, so that you only report worker batches where any messages were indexed.

Since the lowest possible value is 1, rather than 0, will the 0-sample points get discarded?  Maybe you should use a unit of "messages per 10secs" in order to get more detail at the low end so pathological cases stick out more.

Comment 5

[(dormant account)]

Comment on attachment 555657 [details] [diff] [review]
Toolkit patch v1

>-  if (NS_FAILED(rv))
>-    return NS_ERROR_FAILURE;
>+  NS_ENSURE_SUCCESS(rv, rv);

Good catch, but please change this to "return rv"

>- * This file lists Telemetry histograms collected by Firefox.  The format is
>+ * This file lists Telemetry histograms collected by Firefox and other programs.
>+ * The format is

This file lists Telemetry histograms collected by Mozilla?

other programs is pretty vague.

r+ with above return + wording chage

Comment 6

[Jonathan Protzenko [:protz]]

(In reply to Andrew Sutherland (:asuth) from comment #4)
> Comment on attachment 555658 [details] [diff] [review]
> Thunderbird patch v1
> 
> This looks fine, although there may be some ambiguity between whether we are
> just indexing messages really slowly or are working some other type of jobs.
> It might be worth adding another flag that is "_messageIndexingJobActive"
> that gets set when we create a worker for message indexing, so that you only
> report worker batches where any messages were indexed.
> 
> Since the lowest possible value is 1, rather than 0, will the 0-sample
> points get discarded?  Maybe you should use a unit of "messages per 10secs"
> in order to get more detail at the low end so pathological cases stick out
> more.

So the only worker that bumps the _indexedMessageCount is the folder-message-indexer, and there's an if that says "if we didn't index any message, don't add a probe". I guess that answers the concern.

Comment 7

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1af35a19b87e for the first patch only, since the second one is for comm-central

Comment 8

[Justin Wood (:Callek)]

(In reply to Jonathan Protzenko [:protz] from comment #7)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/1af35a19b87e for the
> first patch only, since the second one is for comm-central

Landed on m-c

Comment 9

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/d7fd7d9740f8 for the Thunderbird part