661574 - Telemetry: Create a histogram directory

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[(dormant account)]

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.

Comment 1

[(dormant account)]

Attachment: move nsITelemetry to toolkit

Comment 2

[(dormant account)]

Attachment: telemetry directory wip

Comment 3

[(dormant account)]

Attachment: telemetry directory c++ side(this compiles)

Comment 4

[(dormant account)]

Attachment: js side

another wip

Comment 5

[(dormant account)]

Comment on attachment 537033 [details] [diff] [review]
js side

nevermind, messed up my mq

Comment 6

[(dormant account)]

Attachment: telemetry directory(almost done)

Comment 7

[(dormant account)]

Attachment: part1: move nsITelemetry to toolkit

Comment 8

[(dormant account)]

Attachment: Telemetry directory

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.

Comment 9

[(dormant account)]

Attachment: update zip probes

Comment 10

[(dormant account)]

Attachment: part1: move nsITelemetry to toolkit

This passes try

Comment 11

[(dormant account)]

Attachment: part 2: Telemetry directory

This passes try

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 538293 [details] [diff] [review]
update zip probes

This is nice.

Comment 13

[Dave Townsend [:mossop]]

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?

Comment 14

[(dormant account)]

(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

[Dave Townsend [:mossop]]

(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

Comment 16

[(dormant account)]

Comment on attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

I'll aim for 2/3 r+s for this.

Comment 17

[Michael Wu [:mwu]]

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

Comment 18

[Mike Hommey [:glandium]]

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?

Comment 19

[(dormant account)]

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.

Comment 20

[(dormant account)]

Attachment: part 2: Telemetry directory

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.

Comment 21

[(dormant account)]

Attachment: Part 3: Use a hashtable for id lookup

Comment 22

[(dormant account)]

Attachment: part 4: Get rid of unique telemetry names in favour of ids

might as well get rid of probe names in this bug

Comment 23

[Michael Wu [:mwu]]

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

Comment 24

[(dormant account)]

(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

[Mike Hommey [:glandium]]

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.

Comment 26

[(dormant account)]

> 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

[Mike Hommey [:glandium]]

(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

[Mike Hommey [:glandium]]

(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

[Mike Hommey [:glandium]]

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.

Comment 30

[Michael Wu [:mwu]]

(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.

Comment 31

[(dormant account)]

(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

[Michael Wu [:mwu]]

(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

[Mike Hommey [:glandium]]

I'm not convinced we need to make it alreadyAddrefed either. And if we don't, the dance in bug 661573 is not necessary.

Comment 34

[(dormant account)]

Attachment: Part 3: Use a hashtable for id lookup

Added Clear to the destructor. Also initialize the hashtable to the number of elements in the enum.

Comment 35

[(dormant account)]

(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

[Mike Hommey [:glandium]]

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

Comment 37

[(dormant account)]

Attachment: part 2: Telemetry directory

Got rid of the pointless global that mwu/glandium pointed out.

Comment 38

[(dormant account)]

Attachment: Part 3: Use a hashtable for id lookup

Updated for changes in part 2

Comment 39

[(dormant account)]

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

Comment 40

[Matt Brubeck (:mbrubeck)]

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