Closed Bug 715927 Opened 12 years ago Closed 12 years ago

Do addon telemetry (= stop requiring application-specific defines in TelemetryHistograms.h)

Categories

(Toolkit :: Telemetry, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: sgautherie, Assigned: froydnj)

References

()

Details

Attachments

(4 files, 8 obsolete files)

12.46 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.63 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.08 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.16 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
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
Depends on: 710562
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.
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.
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)
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.
(In reply to Taras Glek (:taras) from comment #4)
> I think it'll be simpler to do
...

+1
Assignee: nobody → nfroyd
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?
Attachment #590336 - Flags: feedback?(bmcbride)
These are tests for the interface itself.
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.
Attachment #590338 - Attachment is patch: true
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.");
(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."); ;->
(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 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.
Attachment #590336 - Flags: feedback?(bmcbride) → feedback+
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.
Attachment #590336 - Attachment is obsolete: true
Attachment #591516 - Flags: review?(taras.mozilla)
Updated with input from sgauthrie.  I noticed we already have an expect_fail function, so used that, and wrote its equivalent expect_success function.
Attachment #590338 - Attachment is obsolete: true
Attachment #591517 - Flags: review?(taras.mozilla)
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.
Attachment #591520 - Flags: review?(taras.mozilla)
Reporting addon histograms, nothing surprising here.  I snuck in an Emacs modeline to TelemetryPing.js, though. :)
Attachment #591522 - Flags: review?(taras.mozilla)
(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 :-)
Status: NEW → ASSIGNED
(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.
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.
Blocks: 721119
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.
(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.)
> 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!
(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 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.
Attachment #591516 - Flags: review?(taras.mozilla) → review+
Attachment #591517 - Flags: review?(taras.mozilla) → review+
Attachment #591522 - Flags: review?(taras.mozilla) → review+
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.
Attachment #591520 - Flags: review?(taras.mozilla) → review+
Attached patch part 5 - C++ template magic (obsolete) — Splinter Review
(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.
Attachment #591884 - Flags: feedback?(taras.mozilla)
Attached patch part 5 - C++ template magic (obsolete) — Splinter Review
Actually, here; I forgot about the template enumeration in TelemetrySessionData.  That got rid of more stuff; the diffstat is now neutral, save for comments.
Attachment #591884 - Attachment is obsolete: true
Attachment #591884 - Flags: feedback?(taras.mozilla)
Attachment #591915 - Flags: feedback?(taras.mozilla)
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.
Attachment #591915 - Flags: feedback?(taras.mozilla)
Attachment #591915 - Flags: feedback?(mh+mozilla)
Attachment #591915 - Flags: feedback+
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.
Attachment #591915 - Flags: feedback?(mh+mozilla) → feedback+
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.
Depends on: 725699
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

Bug 725699 implemented these bits.
Attachment #591915 - Attachment is obsolete: true
Rebasing against trunk.  Carrying over r+.
Attachment #591516 - Attachment is obsolete: true
Attachment #597013 - Flags: review+
Rebasing against trunk.  Carrying over r+.
Attachment #591517 - Attachment is obsolete: true
Attachment #597014 - Flags: review+
Rebasing against trunk.  Carrying over r+.
Attachment #591520 - Attachment is obsolete: true
Attachment #597015 - Flags: review+
Rebasing against trunk.  Carrying over r+.
Attachment #591522 - Attachment is obsolete: true
Attachment #597016 - Flags: review+
Attachment #597013 - Attachment is patch: true
Keywords: checkin-needed
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
(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?
Flags: in-testsuite+
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...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: