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)
Toolkit
Telemetry
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
Comment 1•12 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•12 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•12 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•12 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.
Comment 5•12 years ago
|
||
(In reply to Taras Glek (:taras) from comment #4) > I think it'll be simpler to do ... +1
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → nfroyd
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
These are tests for the interface itself.
Assignee | ||
Comment 8•12 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•12 years ago
|
Attachment #590338 -
Attachment is patch: true
Reporter | ||
Comment 9•12 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•12 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•12 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 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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•12 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
Comment 20•12 years ago
|
||
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•12 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.)
Comment 22•12 years ago
|
||
> 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•12 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•12 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•12 years ago
|
Attachment #591517 -
Flags: review?(taras.mozilla) → review+
Updated•12 years ago
|
Attachment #591522 -
Flags: review?(taras.mozilla) → review+
Comment 25•12 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•12 years ago
|
||
(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•12 years ago
|
||
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•12 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 29•12 years ago
|
||
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•12 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 | ||
Comment 31•12 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•12 years ago
|
||
Rebasing against trunk. Carrying over r+.
Attachment #591516 -
Attachment is obsolete: true
Attachment #597013 -
Flags: review+
Assignee | ||
Comment 33•12 years ago
|
||
Rebasing against trunk. Carrying over r+.
Attachment #591517 -
Attachment is obsolete: true
Attachment #597014 -
Flags: review+
Assignee | ||
Comment 34•12 years ago
|
||
Rebasing against trunk. Carrying over r+.
Attachment #591520 -
Attachment is obsolete: true
Attachment #597015 -
Flags: review+
Assignee | ||
Comment 35•12 years ago
|
||
Rebasing against trunk. Carrying over r+.
Attachment #591522 -
Attachment is obsolete: true
Attachment #597016 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #597013 -
Attachment is patch: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: [autoland-try]
Updated•12 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 36•12 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•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
Comment 38•12 years ago
|
||
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
Comment 39•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 40•12 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 41•12 years ago
|
||
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.
Description
•