Closed
Bug 681873
Opened 13 years ago
Closed 13 years ago
Add basic Telemetry support to Thunderbird
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: protz, Assigned: protz)
References
Details
Attachments
(2 files)
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #555657 -
Flags: review?(bugmail)
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #555658 -
Flags: review?(bugmail)
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Version: 6 Branch → Trunk
Comment 3•13 years ago
|
||
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.
Attachment #555657 -
Flags: review?(bugmail) → review?(tglek)
Comment 4•13 years ago
|
||
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.
Attachment #555658 -
Flags: review?(bugmail) → review+
Comment 5•13 years ago
|
||
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
Attachment #555657 -
Flags: review?(tglek) → review+
Assignee | ||
Comment 6•13 years ago
|
||
(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.
Assignee | ||
Comment 7•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/1af35a19b87e for the first patch only, since the second one is for comm-central
Flags: in-testsuite-
Target Milestone: --- → mozilla9
Comment 8•13 years ago
|
||
(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
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/comm-central/rev/d7fd7d9740f8 for the Thunderbird part
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Assignee: nobody → jonathan.protzenko
You need to log in
before you can comment on or make changes to this bug.
Description
•