Last Comment Bug 715927 - Do addon telemetry (= stop requiring application-specific defines in TelemetryHistograms.h)
: Do addon telemetry (= stop requiring application-specific defines in Telemetr...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 710562 725699
Blocks: 717046 721119
  Show dependency treegraph
 
Reported: 2012-01-06 09:11 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-19 03:10 PST (History)
10 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - C++ bits for addon telemetry (11.69 KB, patch)
2012-01-20 13:59 PST, Nathan Froyd [:froydnj]
blair: feedback+
Details | Diff | Splinter Review
part 2 - tests for addon telemetry (4.55 KB, patch)
2012-01-20 14:00 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
part 1 - C++ bits for addon telemetry (13.72 KB, patch)
2012-01-25 09:55 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 2 - tests for addon telemetry (4.57 KB, patch)
2012-01-25 09:56 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 3 - separate out processHistogram (3.86 KB, patch)
2012-01-25 09:58 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 4 - report addon histograms in telemetry pings (2.15 KB, patch)
2012-01-25 09:59 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 5 - C++ template magic (10.63 KB, patch)
2012-01-26 12:01 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
part 5 - C++ template magic (14.16 KB, patch)
2012-01-26 13:22 PST, Nathan Froyd [:froydnj]
taras.mozilla: feedback+
mh+mozilla: feedback+
Details | Diff | Splinter Review
part 1 - C++ bits for addon telemetry (12.46 KB, patch)
2012-02-14 07:10 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 2 - tests for addon telemetry (4.63 KB, patch)
2012-02-14 07:11 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - separate out packHistogram (4.08 KB, patch)
2012-02-14 07:11 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 4 - report addon histograms in telemetry pings (2.16 KB, patch)
2012-02-14 07:12 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-06 09:11:50 PST
TelemetryHistograms.h issue worked around in bug 710562, for MOZ_PHOENIX and MOZ_THUNDERBIRD.

***

Need a better long-term solution:

(In reply to Taras Glek (:taras) from bug 710562 comment #3)
> If you want a runtime solution, we'll have to wait until we do addon
> telemetry
Comment 1 (dormant account) 2012-01-06 14:56:28 PST
Basically we need to be able to register histograms from a more dynamic source than the precompiled enum. My preferences is for addon authors to be able to specify a list of telemetry probes in some addon metadata.

Not sure how that would translate to xul apps.
Comment 2 (dormant account) 2012-01-06 14:57:50 PST
I do not want a fully-dynamic way to define histograms. I would like to be able to opt-in into non-core histograms in the telemetry frontend. We can't do this unless we can group histograms by addon/application id.
Comment 3 Nathan Froyd [:froydnj] 2012-01-11 08:26:47 PST
We were talking about this in #perf yesterday and the desired interface is something like:

registerAddonId(id, histogram-list)
getAddonHistogram(id, histogram-name)
/* Can unload addons without restart.  */
unregisterAddonId(id)
Comment 4 (dormant account) 2012-01-11 11:15:56 PST
bikeshed:
I think it'll be simpler to do
registerAddonHistogram(id, hgram-name,...) <--allow registering one histogram at a time
getHistogramByAddon(addon-id, hgram-name)
unregisterAddon(id) <-- but do a bulk unregister.

Specifying id in the method is redundant.
Comment 5 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-11 18:53:27 PST
(In reply to Taras Glek (:taras) from comment #4)
> I think it'll be simpler to do
...

+1
Comment 6 Nathan Froyd [:froydnj] 2012-01-20 13:59:51 PST
Created attachment 590336 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

Here's a first cut at addon telemetry.  This is the C++ side of things.  Does this look sufficient for your purposes?  Do we need to worry about addons maliciously unregistering histograms for other addons?
Comment 7 Nathan Froyd [:froydnj] 2012-01-20 14:00:31 PST
Created attachment 590338 [details] [diff] [review]
part 2 - tests for addon telemetry

These are tests for the interface itself.
Comment 8 Nathan Froyd [:froydnj] 2012-01-20 14:01:30 PST
I haven't done the TelemetryPing bits yet; they're fairly trivial: just groveling over the returned snapshots, putting them into the proper format, and sticking them in the ping packet.
Comment 9 Serge Gautherie (:sgautherie) 2012-01-20 17:15:53 PST
Comment on attachment 590338 [details] [diff] [review]
part 2 - tests for addon telemetry

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

In case this test throws/aborts, is it possible to run it again (manually), or would it need to make sure to clean its "data" up?

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +95,5 @@
>  
> +function should_throw(f) {
> +  try {
> +    f();
> +    do_throw("This can't happen");

Nit: use do_throw("Calling f() should not have succeeded.");

Do you actually want this inside try+catch, or after it?

@@ +96,5 @@
> +function should_throw(f) {
> +  try {
> +    f();
> +    do_throw("This can't happen");
> +  } catch (e) {

Nit: add a do_check_*(...) in catch(e) block.

@@ +103,5 @@
> +}
> +
> +function should_not_throw(f) {
> +  try {
> +    f();

Nit: add a do_check_*(...) after calling f().

@@ +105,5 @@
> +function should_not_throw(f) {
> +  try {
> +    f();
> +  } catch (e) {
> +    do_throw("This can't happen");

Nit: use do_throw("Calling f() should have succeeded.");
Comment 10 Serge Gautherie (:sgautherie) 2012-01-20 17:19:35 PST
(In reply to Serge Gautherie (:sgautherie) from comment #9)
> Nit: use do_throw("Calling f() should not have succeeded.");

Or do_throw("Calling f() should have failed."); ;->
Comment 11 Nathan Froyd [:froydnj] 2012-01-21 05:35:30 PST
(In reply to Serge Gautherie (:sgautherie) from comment #9)
> In case this test throws/aborts, is it possible to run it again (manually),
> or would it need to make sure to clean its "data" up?

I think you'd need to clean up, but I'm not sure what the value of running it manually would be.

> ::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
> @@ +95,5 @@
> >  
> > +function should_throw(f) {
> > +  try {
> > +    f();
> > +    do_throw("This can't happen");
> 
> Nit: use do_throw("Calling f() should not have succeeded.");
> 
> Do you actually want this inside try+catch, or after it?

I want it inside; if I put it outside the try+catch, it'd execute all the time.

> @@ +96,5 @@
> > +function should_throw(f) {
> > +  try {
> > +    f();
> > +    do_throw("This can't happen");
> > +  } catch (e) {
> 
> Nit: add a do_check_*(...) in catch(e) block.

do_check_* what?  Have a boolean success variable somewhere that gets set appropriately?  I don't understand what the value of the do_check_* would be.

> @@ +103,5 @@
> > +}
> > +
> > +function should_not_throw(f) {
> > +  try {
> > +    f();
> 
> Nit: add a do_check_*(...) after calling f().

Same question.
Comment 12 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-22 00:04:09 PST
Comment on attachment 590336 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

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

Looks good :)

> Do we need to worry about
> addons maliciously unregistering histograms for other addons?

We can't enforce that here. But we can make it part of the review process for addons submitted to AMO - filed bug 720196 to have AMO's validation process flag usage of these APIs.

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +142,5 @@
> +   * @param name - Unique histogram name
> +   * @param min - Minimal bucket size
> +   * @param max - Maximum bucket size
> +   * @param bucket_count - number of buckets in the histogram.
> +   * @param type - HISTOGRAM_EXPONENTIAL or HISTOGRAM_LINEAR

Can this not be HISTROGRAM_BOOLEAN too?

@@ +144,5 @@
> +   * @param max - Maximum bucket size
> +   * @param bucket_count - number of buckets in the histogram.
> +   * @param type - HISTOGRAM_EXPONENTIAL or HISTOGRAM_LINEAR
> +   */
> +  void registerAddonHistogram(in ACString id, in ACString name,

Could you rename the parameter to "addon_id" for all these functions, makes it more obvious that it's not a histogram ID.

@@ +169,5 @@
> +   * an error if the id has never had histograms associated with it.
> +   *
> +   * @param id - Unique ID of the addon
> +   */
> +  bool unregisterAddonHistograms(in ACString id);

Throwing and returning a boolean is redundant - one or the other. It will be rare to ever care about accidentally unregistering an addon that was never registered, so I think it should be just returning the boolean, and never throw.
Comment 13 Nathan Froyd [:froydnj] 2012-01-25 09:55:48 PST
Created attachment 591516 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

Updated patch with Unfocsed's feedback.  I made unregisterAddonHistograms just return void and not throw errors--do we really care if something tried to unregister when there were no histograms anyway?  Doubt it.

Also cleaned up a few bits in nsITelemetry.idl while I was there.
Comment 14 Nathan Froyd [:froydnj] 2012-01-25 09:56:59 PST
Created attachment 591517 [details] [diff] [review]
part 2 - tests for addon telemetry

Updated with input from sgauthrie.  I noticed we already have an expect_fail function, so used that, and wrote its equivalent expect_success function.
Comment 15 Nathan Froyd [:froydnj] 2012-01-25 09:58:39 PST
Created attachment 591520 [details] [diff] [review]
part 3 - separate out processHistogram

If we're going to send addon histograms, we need the massaging processHistograms() split out of getHistograms so both normal and addon histograms can use it.  I now (sort of) understand what it's trying to do, so there's a big fat comment that goes with it.

Not totally sure why we don't just massage in C++, but this is what we have.
Comment 16 Nathan Froyd [:froydnj] 2012-01-25 09:59:36 PST
Created attachment 591522 [details] [diff] [review]
part 4 - report addon histograms in telemetry pings

Reporting addon histograms, nothing surprising here.  I snuck in an Emacs modeline to TelemetryPing.js, though. :)
Comment 17 Serge Gautherie (:sgautherie) 2012-01-25 10:23:47 PST
(In reply to Nathan Froyd (:froydnj) from comment #11)

> I think you'd need to clean up, but I'm not sure what the value of running
> it manually would be.

I meant running it multiple times locally.
If all Telemetry data are stored at the profile level, that should be fine.
(None is stored at application level, is it?)

> I want it inside; if I put it outside the try+catch, it'd execute all the
> time.

Right. My bad :-(

> do_check_* what?  Have a boolean success variable somewhere that gets set
> appropriately?  I don't understand what the value of the do_check_* would be.

The value is (only) to track test progress.
Your new patch does that just fine :-)
Comment 18 Nathan Froyd [:froydnj] 2012-01-25 10:32:27 PST
(In reply to Serge Gautherie (:sgautherie) from comment #17)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > I think you'd need to clean up, but I'm not sure what the value of running
> > it manually would be.
> 
> I meant running it multiple times locally.
> If all Telemetry data are stored at the profile level, that should be fine.
> (None is stored at application level, is it?)

Any information Telemetry stores is at the profile level only.  For the tests, we don't store any persistent data from test run to test run anyway, though.

You do tangentially raise a good point, though: we could persist addon histograms through shutdown, the same way we persist the registered histograms already.  Will file a bug on that.
Comment 19 Nathan Froyd [:froydnj] 2012-01-25 10:35:00 PST
My understanding is that we have resisted tracking installed addons out of privacy concerns.  If this lands, we can implicitly track addons that users have installed via the histograms the addons generate--but only those addons that use histograms and stuff useful data in them.  Flagging this as privacy-review-needed for possible discussion and/or clarification on this.
Comment 20 Justin Lebar (not reading bugmail) 2012-01-25 10:59:33 PST
Privacy concerns aside, this kind of tracking is critical to current MemShrink (and likely snappy) efforts; this telemetry will help us track down add-ons which misbehave by leaking memory or otherwise causing Firefox to suck.
Comment 21 Nathan Froyd [:froydnj] 2012-01-25 11:09:58 PST
(In reply to Justin Lebar [:jlebar] from comment #20)
> Privacy concerns aside, this kind of tracking is critical to current
> MemShrink (and likely snappy) efforts; this telemetry will help us track
> down add-ons which misbehave by leaking memory or otherwise causing Firefox
> to suck.

Please note--and you may already know this, just making it explicit--that you'll just be able to get information about addons that use histograms, and addons have to opt-in to doing so.  You can at least correlate high memory usage with said addons, at least.  (I'd wager that addons using histograms are not going to be the addons causing the problems, though.)
Comment 22 Justin Lebar (not reading bugmail) 2012-01-25 11:20:05 PST
> My understanding is that we have resisted tracking installed addons out of privacy concerns.

Taras pointed me to the code in the tree which sends the list of add-ons: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#287

Thanks for clarifying what this bug is about!
Comment 23 Nathan Froyd [:froydnj] 2012-01-25 11:26:54 PST
(In reply to Justin Lebar [:jlebar] from comment #22)
> > My understanding is that we have resisted tracking installed addons out of privacy concerns.
> 
> Taras pointed me to the code in the tree which sends the list of add-ons:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryPing.js#287
> 
> Thanks for clarifying what this bug is about!

Thanks for pointing that out!  I guess we're in the clear here, then.
Comment 24 (dormant account) 2012-01-25 14:39:13 PST
Comment on attachment 591516 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

I see about 3 existing variations of 
+  struct AddonEnumeratorArgs {
+    JSContext *cx;
+    JSObject *obj;
+  };

Please make them one. 

Rant: I wonder if we can do some fancy c++ to reduce amount of hashtable boilerplate, this is getting insane. This is optional.

+void
+AddonHistogramName(const nsACString &id, const nsACString &name,

-> GetAddonHistogramName

Seems ok.
Comment 25 (dormant account) 2012-01-25 16:17:54 PST
Comment on attachment 591520 [details] [diff] [review]
part 3 - separate out processHistogram

please get rid of redundant 
first = false;
that i left in. 

* We want to compress the data somewhat--in particular, we want to
* not send 0 buckets, as there might be quite a lot of those.

That's not quite true, the code explicitly adds 0 buckets in the beginning an and to indicate lower/upper bounds of the histogram. This is just an aid to making reading raw histograms easier.

convertHistogram is a bad name, use reduce, pickle, package, or anything else that better describes converting from inmemory format to wire format.
Comment 26 Nathan Froyd [:froydnj] 2012-01-26 12:01:56 PST
Created attachment 591884 [details] [diff] [review]
part 5 - C++ template magic

(In reply to Taras Glek (:taras) from comment #24)
> Comment on attachment 591516 [details] [diff] [review]
> part 1 - C++ bits for addon telemetry
> 
> I see about 3 existing variations of 
> +  struct AddonEnumeratorArgs {
> +    JSContext *cx;
> +    JSObject *obj;
> +  };
> 
> Please make them one. 
> 
> Rant: I wonder if we can do some fancy c++ to reduce amount of hashtable
> boilerplate, this is getting insane. This is optional.

What do you think about this patch?  It does make hashtable slightly slower (one extra function call per entry), but the actual functions for reflecting hashtable entries don't have to do argument unpacking and the like.

You could also just have ReflectHistograms with a standard nsTHashtable enumerator function and continue to let the individual enumerator functions unpack arguments and manage the hashtable iteration directly.

The diffstat on this patch is slightly positive, but that's probably due to the crazy-long function type declarations we have now.
Comment 27 Nathan Froyd [:froydnj] 2012-01-26 13:22:01 PST
Created attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

Actually, here; I forgot about the template enumeration in TelemetrySessionData.  That got rid of more stuff; the diffstat is now neutral, save for comments.
Comment 28 (dormant account) 2012-01-26 13:46:55 PST
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

seems ok. I asked glandium to see if we can get any further reductions with template usage.
Comment 29 Mike Hommey [:glandium] 2012-01-30 07:59:41 PST
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

At a quick glance, it doesn't look like there's much to win from templates, here, except maybe turning entryFunc into a template argument, as a functor. Not sure it's worth.
Comment 30 (dormant account) 2012-02-07 14:02:03 PST
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

one thing this patch doesn't do is a RAII wrapper for nshashtble to do Init/Clear. That might be useful in mfbt or something.
Comment 31 Nathan Froyd [:froydnj] 2012-02-14 07:10:00 PST
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

Bug 725699 implemented these bits.
Comment 32 Nathan Froyd [:froydnj] 2012-02-14 07:10:46 PST
Created attachment 597013 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

Rebasing against trunk.  Carrying over r+.
Comment 33 Nathan Froyd [:froydnj] 2012-02-14 07:11:17 PST
Created attachment 597014 [details] [diff] [review]
part 2 - tests for addon telemetry

Rebasing against trunk.  Carrying over r+.
Comment 34 Nathan Froyd [:froydnj] 2012-02-14 07:11:51 PST
Created attachment 597015 [details] [diff] [review]
part 3 - separate out packHistogram

Rebasing against trunk.  Carrying over r+.
Comment 35 Nathan Froyd [:froydnj] 2012-02-14 07:12:16 PST
Created attachment 597016 [details] [diff] [review]
part 4 - report addon histograms in telemetry pings

Rebasing against trunk.  Carrying over r+.
Comment 36 Mozilla RelEng Bot 2012-02-14 07:17:09 PST
Autoland Patchset:
	Patches: 597013, 597014, 597015, 597016
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=a7da4221df6a
Try run started, revision a7da4221df6a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=a7da4221df6a
Comment 37 Mozilla RelEng Bot 2012-02-14 11:45:28 PST
Try run for a7da4221df6a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a7da4221df6a
Results (out of 211 total builds):
    exception: 1
    success: 179
    warnings: 17
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-a7da4221df6a
Comment 40 Serge Gautherie (:sgautherie) 2012-02-17 08:50:43 PST
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> TelemetryHistograms.h issue worked around in bug 710562, for MOZ_PHOENIX and
> MOZ_THUNDERBIRD.

What is the plan to actually get rid of the existing app-specific defines?
Comment 41 neil@parkwaycc.co.uk 2012-02-19 03:10:09 PST
Comment on attachment 597013 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

>+    nsCAutoString actualName;
>+    AddonHistogramName(id, name, actualName);
>+    nsresult rv = HistogramGet(PromiseFlatCString(actualName).get(),
actualName is already a flat string here...

Note You need to log in before you can comment on or make changes to this bug.