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

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Telemetry
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 8 obsolete attachments)

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
(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 710562

Comment 1

5 years ago
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

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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

5 years ago
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
Blocks: 717046
(Assignee)

Updated

5 years ago
Assignee: nobody → nfroyd
(Assignee)

Comment 6

5 years ago
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?
Attachment #590336 - Flags: feedback?(bmcbride)
(Assignee)

Comment 7

5 years ago
Created attachment 590338 [details] [diff] [review]
part 2 - tests for addon telemetry

These are tests for the interface itself.
(Assignee)

Comment 8

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #590338 - Attachment is patch: true
(Reporter)

Comment 9

5 years ago
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.");
(Reporter)

Comment 10

5 years ago
(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."); ;->
(Assignee)

Comment 11

5 years ago
(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+
(Assignee)

Comment 13

5 years ago
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.
Attachment #590336 - Attachment is obsolete: true
Attachment #591516 - Flags: review?(taras.mozilla)
(Assignee)

Comment 14

5 years ago
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.
Attachment #590338 - Attachment is obsolete: true
Attachment #591517 - Flags: review?(taras.mozilla)
(Assignee)

Comment 15

5 years ago
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.
Attachment #591520 - Flags: review?(taras.mozilla)
(Assignee)

Comment 16

5 years ago
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. :)
Attachment #591522 - Flags: review?(taras.mozilla)
(Reporter)

Comment 17

5 years ago
(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
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
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.
Keywords: privacy-review-needed
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 21

5 years ago
(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!
(Assignee)

Comment 23

5 years ago
(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.
Keywords: privacy-review-needed

Comment 24

5 years ago
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+

Updated

5 years ago
Attachment #591517 - Flags: review?(taras.mozilla) → review+

Updated

5 years ago
Attachment #591522 - Flags: review?(taras.mozilla) → review+

Comment 25

5 years ago
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+
(Assignee)

Comment 26

5 years ago
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.
Attachment #591884 - Flags: feedback?(taras.mozilla)
(Assignee)

Comment 27

5 years ago
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.
Attachment #591884 - Attachment is obsolete: true
Attachment #591884 - Flags: feedback?(taras.mozilla)
Attachment #591915 - Flags: feedback?(taras.mozilla)

Comment 28

5 years ago
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 30

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 725699
(Assignee)

Comment 31

5 years ago
Comment on attachment 591915 [details] [diff] [review]
part 5 - C++ template magic

Bug 725699 implemented these bits.
Attachment #591915 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 597013 [details] [diff] [review]
part 1 - C++ bits for addon telemetry

Rebasing against trunk.  Carrying over r+.
Attachment #591516 - Attachment is obsolete: true
Attachment #597013 - Flags: review+
(Assignee)

Comment 33

5 years ago
Created attachment 597014 [details] [diff] [review]
part 2 - tests for addon telemetry

Rebasing against trunk.  Carrying over r+.
Attachment #591517 - Attachment is obsolete: true
Attachment #597014 - Flags: review+
(Assignee)

Comment 34

5 years ago
Created attachment 597015 [details] [diff] [review]
part 3 - separate out packHistogram

Rebasing against trunk.  Carrying over r+.
Attachment #591520 - Attachment is obsolete: true
Attachment #597015 - Flags: review+
(Assignee)

Comment 35

5 years ago
Created attachment 597016 [details] [diff] [review]
part 4 - report addon histograms in telemetry pings

Rebasing against trunk.  Carrying over r+.
Attachment #591522 - Attachment is obsolete: true
Attachment #597016 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #597013 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 36

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
http://hg.mozilla.org/integration/mozilla-inbound/rev/2389419e7c08
http://hg.mozilla.org/integration/mozilla-inbound/rev/ffaae77565fe
http://hg.mozilla.org/integration/mozilla-inbound/rev/60a2ab351c58
http://hg.mozilla.org/integration/mozilla-inbound/rev/c83ba397936f
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/2389419e7c08
https://hg.mozilla.org/mozilla-central/rev/ffaae77565fe
https://hg.mozilla.org/mozilla-central/rev/60a2ab351c58
https://hg.mozilla.org/mozilla-central/rev/c83ba397936f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 40

5 years ago
(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.