Closed Bug 661574 Opened 9 years ago Closed 8 years ago

Telemetry: Create a histogram directory

Categories

(Toolkit :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 11 obsolete files)

2.43 KB, patch
mwu
: review+
Details | Diff | Splinter Review
3.32 KB, patch
mossop
: review+
Details | Diff | Splinter Review
4.05 KB, patch
glandium
: review+
Details | Diff | Splinter Review
22.29 KB, patch
taras.mozilla
: review+
Details | Diff | Splinter Review
3.04 KB, 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.
Attached patch move nsITelemetry to toolkit (obsolete) — Splinter Review
Assignee: nobody → tglek
Attached patch telemetry directory wip (obsolete) — Splinter Review
Attachment #537008 - Attachment is obsolete: true
Attached patch js side (obsolete) — Splinter Review
another wip
Comment on attachment 537033 [details] [diff] [review]
js side

nevermind, messed up my mq
Attachment #537033 - Attachment is obsolete: true
Blocks: 661881
Attached patch telemetry directory(almost done) (obsolete) — Splinter Review
Attachment #537007 - Attachment is obsolete: true
Attachment #537024 - Attachment is obsolete: true
Attachment #537267 - Attachment is obsolete: true
Attachment #538047 - Flags: review?(dtownsend)
Attached patch Telemetry directory (obsolete) — Splinter Review
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)
Attachment #538293 - Flags: review?(mwu)
This passes try
Attachment #538047 - Attachment is obsolete: true
Attachment #538047 - Flags: review?(dtownsend)
Attachment #538296 - Flags: review?(dtownsend)
Attached patch part 2: Telemetry directory (obsolete) — Splinter Review
This passes try
Attachment #538122 - Attachment is obsolete: true
Attachment #538122 - Flags: review?(dtownsend)
Attachment #538297 - Flags: review?(dtownsend)
Comment on attachment 538293 [details] [diff] [review]
update zip probes

This is nice.
Attachment #538293 - Flags: review?(mwu) → review+
Attachment #538296 - Flags: review?(dtownsend) → review+
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)
(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?
(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
Blocks: 663105
Blocks: 663920
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)
Blocks: 661573
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 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+
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.
Attached patch part 2: Telemetry directory (obsolete) — Splinter Review
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+
Attachment #539623 - Flags: review?(mh+mozilla)
might as well get rid of probe names in this bug
Attachment #539624 - Flags: review?(mh+mozilla)
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
(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 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+
> 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?
(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.
(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 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+
(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.
(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
(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.
I'm not convinced we need to make it alreadyAddrefed either. And if we don't, the dance in bug 661573 is not necessary.
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+
(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
(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)
Got rid of the pointless global that mwu/glandium pointed out.
Attachment #539622 - Attachment is obsolete: true
Attachment #540194 - Flags: review+
Updated for changes in part 2
Attachment #539904 - Attachment is obsolete: true
Attachment #540195 - Flags: review+
Blocks: 665805
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.