Closed
Bug 661574
Opened 13 years ago
Closed 13 years ago
Telemetry: Create a histogram directory
Categories
(Toolkit :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Keywords: dev-doc-complete)
Attachments
(5 files, 11 obsolete files)
(deleted),
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
Currently telemetry histograms are instantiated in an ad-hoc manner. Having a centralized directory would make it easier to document and do security review of new data points.
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 5•13 years ago
|
||
Comment on attachment 537033 [details] [diff] [review] js side nevermind, messed up my mq
Attachment #537033 -
Attachment is obsolete: true
Assignee | ||
Comment 6•13 years ago
|
||
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #537007 -
Attachment is obsolete: true
Attachment #537024 -
Attachment is obsolete: true
Attachment #537267 -
Attachment is obsolete: true
Attachment #538047 -
Flags: review?(dtownsend)
Assignee | ||
Comment 8•13 years ago
|
||
This patch ended up a bit bigger than I hoped, but most of it mechanical changes. I renamed Telemetry -> TelemetryImpl, introduced a Telemetry namespace so telemetry can be used from C++ code(as opposed to using the chrome macros directly). I added Telemetry::Accumulate(id, value) for C++ and a getHistogramById(id) to do telemetry stuff from js side. The APIs diverge slightly because this allows me to not leak chrome classes/headers into the C++ side, making it easier to utilize telemetry. The JS side is the way it is to be consistent with existing APIs. Once this lands using chrome macros directly will be forbidden to ease security/privacy reviews(ie so they only have to look at the centralized list). In the future, I plan to add a way to iterate the histogram list for about:telemetry.
Attachment #538122 -
Flags: review?(dtownsend)
Assignee | ||
Comment 10•13 years ago
|
||
This passes try
Attachment #538047 -
Attachment is obsolete: true
Attachment #538047 -
Flags: review?(dtownsend)
Attachment #538296 -
Flags: review?(dtownsend)
Assignee | ||
Comment 11•13 years ago
|
||
This passes try
Attachment #538122 -
Attachment is obsolete: true
Attachment #538122 -
Flags: review?(dtownsend)
Attachment #538297 -
Flags: review?(dtownsend)
Comment 12•13 years ago
|
||
Comment on attachment 538293 [details] [diff] [review] update zip probes This is nice.
Attachment #538293 -
Flags: review?(mwu) → review+
Updated•13 years ago
|
Attachment #538296 -
Flags: review?(dtownsend) → review+
Comment 13•13 years ago
|
||
Comment on attachment 538297 [details] [diff] [review] part 2: Telemetry directory To my eye it looks ok but I'd prefer to have someone with more working C++ than me to take a look over. Shawn, would you have some time to look at this reasonably soon? A couple of comments, don't think any of these are really required though: * Seems like you could use a nsDataHashtable mHistogramMap which can handle nsCString keys rather than needing to jump to a char* * Having both ID and name map to a histogram becomes a little confusing, any strong reason not to just always map from name?
Attachment #538297 -
Flags: review?(dtownsend) → review?(sdwilsh)
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to comment #13) > Comment on attachment 538297 [details] [diff] [review] [review] > part 2: Telemetry directory > > To my eye it looks ok but I'd prefer to have someone with more working C++ > than me to take a look over. Shawn, would you have some time to look at this > reasonably soon? > > A couple of comments, don't think any of these are really required though: > > * Seems like you could use a nsDataHashtable mHistogramMap which can handle > nsCString keys rather than needing to jump to a char* I chose char* so I could point at static strings. I don't see much benefit from switching to an xpcom datastructure here. The one string convertion pales in comparison to overhead of xpconnect call. > * Having both ID and name map to a histogram becomes a little confusing, any > strong reason not to just always map from name? Performance. Doing probes from C++ is just a few function calls and an array lookup ie O(1), vs logN for js lookup. Dave/Shawn, is it ok if I get glandium/mwu to review the C++ bits if Shawn can't get to this soon?
Comment 15•13 years ago
|
||
(In reply to comment #14) > > * Having both ID and name map to a histogram becomes a little confusing, any > > strong reason not to just always map from name? > > Performance. Doing probes from C++ is just a few function calls and an array > lookup ie O(1), vs logN for js lookup. > > Dave/Shawn, is it ok if I get glandium/mwu to review the C++ bits if Shawn > can't get to this soon? Yeah that'd be fine
Assignee | ||
Comment 16•13 years ago
|
||
Comment on attachment 538297 [details] [diff] [review] part 2: Telemetry directory I'll aim for 2/3 r+s for this.
Attachment #538297 -
Flags: superreview?(mh+mozilla)
Attachment #538297 -
Flags: review?(mwu)
Comment 17•13 years ago
|
||
Comment on attachment 538297 [details] [diff] [review] part 2: Telemetry directory Review of attachment 538297 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine overall. Only thing I really care about is the use of abstract strings. The rest are nits. ::: toolkit/components/telemetry/Telemetry.cpp @@ +58,3 @@ > > +struct ltstr > +{ Put { on the same line as struct for consistency. @@ +72,5 @@ > public: > + static TelemetryImpl* GetSingleton(); > + > +private: > + // This is used to cache JS string->TelemetryImpl::ID conversions Telemetry::ID? @@ +88,5 @@ > + const char *name; > + PRUint32 min; > + PRUint32 max; > + PRUint32 bucket_count; > + PRUint32 histogram_type; Maybe camelCase here for consistency with other mozilla things? @@ +93,5 @@ > +}; > + > +const TelemetryHistogram gHistograms[] = { > +#define HISTOGRAM(id, name, min, max, bucket_count, histogram_type, b) \ > + {NULL, NS_STRINGIFY(id), name, min, max, bucket_count, nsITelemetry::HISTOGRAM_ ## histogram_type}, Space after { and before } @@ +102,5 @@ > +}; > + > +nsresult > +HistogramGet(const char *name, PRUint32 min, PRUint32 max, PRUint32 bucket_count, > + PRUint32 histogram_type, Histogram **aResult) This is sorta weird mixing camelCase and c_style. Also odd that aResult is the only arg that's prefixed. @@ +134,5 @@ > +} > + > +// O(1) histogram lookup by numeric id > +nsresult > +GetHistogramByEnumId(Telemetry::ID id, Histogram **ret) Looks like you can just return Histogram *? @@ +248,2 @@ > Histogram *h; > + nsresult rv = HistogramGet(name.BeginReading(), min, max, bucket_count, histogram_type, &h); PromiseFlatCString(name).get() @@ +281,5 @@ > +{ > + // Cache names for log(N) lookup > + // Note the histogram names are statically allocated > + if (Telemetry::HistogramCount && !mHistogramMap.size()) { > + for (PRUint32 i = 0;i < Telemetry::HistogramCount;i++) { Space after the ;s @@ +286,5 @@ > + mHistogramMap[gHistograms[i].id] = (Telemetry::ID) i; > + } > + } > + > + NameHistogramMap::iterator it = mHistogramMap.find(name.BeginReading()); PromiseFlatCString(name).get() ::: toolkit/components/telemetry/Telemetry.h @@ +1,1 @@ > +//* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */ //*? ::: toolkit/components/telemetry/TelemetryHistograms.h @@ +1,1 @@ > +//* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */ //*? ::: toolkit/components/telemetry/nsITelemetry.idl @@ +80,5 @@ > [implicit_jscontext] > jsval newHistogram(in ACString name, in PRUint32 min, in PRUint32 max, in PRUint32 bucket_count, in unsigned long histogram_type); > + > + /** > + * Same as newHistogram above, but for histograms registed in TelemetryHistograms.h. registered
Attachment #538297 -
Flags: review?(mwu) → review+
Comment 18•13 years ago
|
||
Comment on attachment 538297 [details] [diff] [review] part 2: Telemetry directory Review of attachment 538297 [details] [diff] [review]: ----------------------------------------------------------------- Just a few nits: - It would be nicer if the addition of the boolean histogram type was an entirely separate patch, but either way is fine for me. ::: toolkit/components/telemetry/Telemetry.cpp @@ +52,2 @@ > > namespace { why not under mozilla::Telemetry here too? @@ +73,5 @@ > + static TelemetryImpl* GetSingleton(); > + > +private: > + // This is used to cache JS string->TelemetryImpl::ID conversions > + typedef map<const char*, Telemetry::ID, ltstr> NameHistogramMap; Any particular reason not to use nsTHashtable? @@ +93,5 @@ > +}; > + > +const TelemetryHistogram gHistograms[] = { > +#define HISTOGRAM(id, name, min, max, bucket_count, histogram_type, b) \ > + {NULL, NS_STRINGIFY(id), name, min, max, bucket_count, nsITelemetry::HISTOGRAM_ ## histogram_type}, This adds relocations for the strings. OTOH, any alternative would be ugly :( ::: toolkit/components/telemetry/TelemetryHistograms.h @@ +38,5 @@ > + > +/** > + * This file lists Telemetry histograms collected by Firefox. > + * Format is HISTOGRAM(id, histogram name, minium, maximum, bucket count, > + * histogram kind, human-readable description for about:telemetry) I'm wondering. Do we really need the histogram name ? Shouldn't the id be enough, with the human-readable description?
Attachment #538297 -
Flags: superreview?(mh+mozilla) → superreview+
Assignee | ||
Comment 19•13 years ago
|
||
Thanks for the quick reviews. > > +nsresult > > +GetHistogramByEnumId(Telemetry::ID id, Histogram **ret) > > Looks like you can just return Histogram *? It's more convenient for one of the callers to do nsresult and as it passes slightly better error messages to JS. (In reply to comment #18) > > - It would be nicer if the addition of the boolean histogram type was an > entirely separate patch, but either way is fine for me. Sorry. > > ::: toolkit/components/telemetry/Telemetry.cpp > @@ +52,2 @@ > > > > namespace { > > why not under mozilla::Telemetry here too? Because toplevel anonymous namespace helps the compiler do "static"-like things > > @@ +73,5 @@ > > + static TelemetryImpl* GetSingleton(); > > + > > +private: > > + // This is used to cache JS string->TelemetryImpl::ID conversions > > + typedef map<const char*, Telemetry::ID, ltstr> NameHistogramMap; > > Any particular reason not to use nsTHashtable? Other than the awkward interface, no reason. Updated patch coming up. > ::: toolkit/components/telemetry/TelemetryHistograms.h > @@ +38,5 @@ > > + > > +/** > > + * This file lists Telemetry histograms collected by Firefox. > > + * Format is HISTOGRAM(id, histogram name, minium, maximum, bucket count, > > + * histogram kind, human-readable description for about:telemetry) > > I'm wondering. Do we really need the histogram name ? Shouldn't the id be > enough, with the human-readable description? You raise a really good point. Using the id gives us a benefit of guaranteeing uniqueness by the compiler(since they have to be valid enum values). I think I'll do that in a follow up. I wanted to do fancy things with formatting in the name, but the description field should be better for that anyway.
Assignee | ||
Comment 20•13 years ago
|
||
Updated part 2 according to review comments. By the way, really appreciate the quality of comments in this review round. Carrying over r+. This also includes a fix for an issue mwu spotted on irc. It renames GetSingleton to GetInstance and makes it alreadyAddrefed. This avoids accidental over-addrefing that I was doing in bug 661573.
Attachment #538297 -
Attachment is obsolete: true
Attachment #538297 -
Flags: review?(sdwilsh)
Attachment #539622 -
Flags: review+
Assignee | ||
Comment 22•13 years ago
|
||
might as well get rid of probe names in this bug
Attachment #539624 -
Flags: review?(mh+mozilla)
Comment 23•13 years ago
|
||
Comment on attachment 539622 [details] [diff] [review] part 2: Telemetry directory Review of attachment 539622 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Telemetry.cpp @@ +307,3 @@ > } > > +already_AddRefed<TelemetryImpl> NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR does this for you afaict - http://hg.mozilla.org/mozilla-central/file/cab0115bbc48/xpcom/components/ModuleUtils.h#l101
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to comment #23) > Comment on attachment 539622 [details] [diff] [review] [review] > part 2: Telemetry directory > > Review of attachment 539622 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/telemetry/Telemetry.cpp > @@ +307,3 @@ > > } > > > > +already_AddRefed<TelemetryImpl> > > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR does this for you afaict - > http://hg.mozilla.org/mozilla-central/file/cab0115bbc48/xpcom/components/ > ModuleUtils.h#l101 The problem is that if one returns a naked pointer in GetSingleton, then one is tempted to do GetSingleton()->foo which is normally a completely sane C++ pattern, but in this case GetSingleton() isn't a simple getter and is addrefing stuff. Wrapping it in already_AddRefed documents that and eliminates accidental misuse. Having said that, if you have any suggestions to GetSingleton/GetInstance dance in bug 661573, I'm all ears.
Comment 25•13 years ago
|
||
Comment on attachment 539623 [details] [diff] [review] Part 3: Use a hashtable for id lookup Review of attachment 539623 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Note this is the typical case where it would be better to use something like gperf, but afaik, we don't do that.
Attachment #539623 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 26•13 years ago
|
||
> Looks good. Note this is the typical case where it would be better to use
> something like gperf, but afaik, we don't do that.
gperf?
Comment 27•13 years ago
|
||
(In reply to comment #25) > Comment on attachment 539623 [details] [diff] [review] [review] > Part 3: Use a hashtable for id lookup > > Review of attachment 539623 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > Looks good. Note this is the typical case where it would be better to use > something like gperf, but afaik, we don't do that. Actually, there is something missing: clearing the hashtable when destroying the TelemetryImpl instance.
Comment 28•13 years ago
|
||
(In reply to comment #26) > > Looks good. Note this is the typical case where it would be better to use > > something like gperf, but afaik, we don't do that. > > gperf? http://www.gnu.org/software/gperf/ a perfect hash generator. It generates the hashtable at build time instead of runtime, which is good for static hashtables like that one.
Comment 29•13 years ago
|
||
Comment on attachment 539624 [details] [diff] [review] part 4: Get rid of unique telemetry names in favour of ids Review of attachment 539624 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryHistograms.h @@ +46,5 @@ > +HISTOGRAM(TELEMETRY_PING, 1, 3000, 10, EXPONENTIAL, "Time(ms) taken to submit telemetry info") > +HISTOGRAM(TELEMETRY_SUCCESS, 0, 1, 2, BOOLEAN, "Success(No, Yes) rate of telemetry submissions") > +HISTOGRAM(MEMORY_JS_GC_HEAP, 1024, 512 * 1024, 10, EXPONENTIAL, "Memory(MB) used by the JavaScript GC") > +HISTOGRAM(MEMORY_RESIDENT, 32 * 1024, 1024 * 1024, 10, EXPONENTIAL, "Resident memory(MB) reported by OS") > +HISTOGRAM(MEMORY_LAYOUT_ALL, 1024, 64 * 1024, 10, EXPONENTIAL, "Memory(MB) reported used by layout") I'd put the units at the end. ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +72,5 @@ > do_check_eq(payload.info[f], expected_info[f]); > } > > + const TELEMETRY_PING = "TELEMETRY_PING"; > + const TELEMETRY_SUCCESS = "TELEMETRY_SUCCESS"; You could just get rid of these entirely and use strings directly (especially considering they're only used once). You also need to modify toolkit/components/telemetry/TelemetryPing.js. Other than that, lgtm.
Attachment #539624 -
Flags: review?(mh+mozilla) → review+
Comment 30•13 years ago
|
||
(In reply to comment #24) > The problem is that if one returns a naked pointer in GetSingleton, then one > is tempted to do GetSingleton()->foo which is normally a completely sane C++ > pattern, but in this case GetSingleton() isn't a simple getter and is > addrefing stuff. Wrapping it in already_AddRefed documents that and > eliminates accidental misuse. Having said that, if you have any suggestions > to GetSingleton/GetInstance dance in bug 661573, I'm all ears. But.. there are no users who are doing GetSingleton()->foo. Are you expecting users who will? It's fine if there will be, but I'd lean towards consistency with other GetSingleton impls if there won't be.
Assignee | ||
Comment 31•13 years ago
|
||
(In reply to comment #30) > But.. there are no users who are doing GetSingleton()->foo. Are you > expecting users who will? It's fine if there will be, but I'd lean towards > consistency with other GetSingleton impls if there won't be. bug 661573
Comment 32•13 years ago
|
||
(In reply to comment #20) > This also includes a fix for an issue mwu spotted on irc. It renames > GetSingleton to GetInstance and makes it alreadyAddrefed. This avoids > accidental over-addrefing that I was doing in bug 661573. The issue I was referring to was the Telemetry code adding its own reference when creating the object. Other code like the cookie service - http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#571 only addref once even if the object is being created for the first time.
Comment 33•13 years ago
|
||
I'm not convinced we need to make it alreadyAddrefed either. And if we don't, the dance in bug 661573 is not necessary.
Assignee | ||
Comment 34•13 years ago
|
||
Added Clear to the destructor. Also initialize the hashtable to the number of elements in the enum.
Attachment #539623 -
Attachment is obsolete: true
Attachment #539904 -
Flags: review+
Assignee | ||
Comment 35•13 years ago
|
||
(In reply to comment #29) > You also need to modify toolkit/components/telemetry/TelemetryPing.js. Are you sure? That was already using ids, unless I'm missing something
Comment 36•13 years ago
|
||
(In reply to comment #35) > (In reply to comment #29) > > > You also need to modify toolkit/components/telemetry/TelemetryPing.js. > > Are you sure? That was already using ids, unless I'm missing something You're right, I missed the change in part 2. (you could still remove the consts from there, though)
Assignee | ||
Comment 37•13 years ago
|
||
Got rid of the pointless global that mwu/glandium pointed out.
Attachment #539622 -
Attachment is obsolete: true
Attachment #540194 -
Flags: review+
Assignee | ||
Comment 38•13 years ago
|
||
Updated for changes in part 2
Attachment #539904 -
Attachment is obsolete: true
Attachment #540195 -
Flags: review+
Assignee | ||
Comment 39•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8c813f7586df http://hg.mozilla.org/integration/mozilla-inbound/rev/aef1c7a2dd3c http://hg.mozilla.org/integration/mozilla-inbound/rev/e12bf65232fd http://hg.mozilla.org/integration/mozilla-inbound/rev/c70b52439e1c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 40•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c70b52439e1c http://hg.mozilla.org/mozilla-central/rev/e12bf65232fd http://hg.mozilla.org/mozilla-central/rev/aef1c7a2dd3c http://hg.mozilla.org/mozilla-central/rev/8c813f7586df
Target Milestone: --- → mozilla7
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•