Add weekly crash counters, refactor metrics_daemon, respect opt-in in library.

Add weekly crash counters, refactor metrics_daemon, respect opt-in in library.

BUG=5340, 5814
Change-Id: I2c207055f1ebe48051193395e2dbe38d9140b025

[petkov, 2010-08-24 06:13:29]

Some nits and a suggestion for some more cleanup.

http://codereview.chromium.org/3171023/diff/4001/5004
File counter.h (right):

http://codereview.chromium.org/3171023/diff/4001/5004#newcode37
counter.h:37: class TaggedCounterInterface {
If you make FrequencyCounter take TaggedCounterReporter, you could get rid of
TaggedCounterInterface completely, I think.

http://codereview.chromium.org/3171023/diff/4001/5004#newcode164
counter.h:164: // TaggedCounterReporter provides
Finish comment.

http://codereview.chromium.org/3171023/diff/4001/5007
File metrics_daemon.cc (right):

http://codereview.chromium.org/3171023/diff/4001/5007#newcode240
metrics_daemon.cc:240: const size_t crash_inteval_configs_count =
Use arraysize here?

http://codereview.chromium.org/3171023/diff/4001/5007#newcode291
metrics_daemon.cc:291:
sizeof(crash_frequency_configs)/sizeof(crash_frequency_configs[0]);
arraysize

http://codereview.chromium.org/3171023/diff/4001/5009
File metrics_daemon_test.cc (right):

http://codereview.chromium.org/3171023/diff/4001/5009#newcode7
metrics_daemon_test.cc:7: #include <utime.h>
make this a separate top include (C headers).

http://codereview.chromium.org/3171023/diff/4001/5009#newcode96
metrics_daemon_test.cc:96: mkdir(kTestDir, 0700);
Isn't there a file_util for creating a directory?

http://codereview.chromium.org/3171023/diff/4001/5010
File metrics_library.cc (right):

http://codereview.chromium.org/3171023/diff/4001/5010#newcode154
metrics_library.cc:154: if (!AreMetricsEnabled())
You could move this inside SendMessageToChrome, and then you wouldn't need it
both here and in SendEnumToUMA.

http://codereview.chromium.org/3171023/diff/4001/5011
File metrics_library.h (right):

http://codereview.chromium.org/3171023/diff/4001/5011#newcode75
metrics_library.h:75: FRIEND_TEST(CMetricsLibraryTest, AreMetricsEnabled);
Do you need this friend?

[petkov, 2010-08-24 16:42:45]

This CL doesn't set epoch based on OS version, right?

http://codereview.chromium.org/3171023/diff/4001/5004
File counter.h (right):

http://codereview.chromium.org/3171023/diff/4001/5004#newcode37
counter.h:37: class TaggedCounterInterface {
On 2010/08/24 06:13:29, petkov wrote:
> If you make FrequencyCounter take TaggedCounterReporter, you could get rid of
> TaggedCounterInterface completely, I think.
> 

I guess you've left it this way so that one could potentially use
FrequencyCounter with normal TaggedCounter?

[kmixter1, 2010-08-25 01:22:17]

PTAL. Responded to comments, added a few more tests, simplified the file name
scheme like we discussed offline.

http://codereview.chromium.org/3171023/diff/4001/5004
File counter.h (right):

http://codereview.chromium.org/3171023/diff/4001/5004#newcode37
counter.h:37: class TaggedCounterInterface {
On 2010/08/24 16:42:45, petkov wrote:
> On 2010/08/24 06:13:29, petkov wrote:
> > If you make FrequencyCounter take TaggedCounterReporter, you could get rid
of
> > TaggedCounterInterface completely, I think.
> > 
> 
> I guess you've left it this way so that one could potentially use
> FrequencyCounter with normal TaggedCounter?

Exactly.

http://codereview.chromium.org/3171023/diff/4001/5004#newcode164
counter.h:164: // TaggedCounterReporter provides
On 2010/08/24 06:13:29, petkov wrote:
> Finish comment.
> 

Ironically I had already seen this and changed it but not uploaded.  See
current.

http://codereview.chromium.org/3171023/diff/4001/5009
File metrics_daemon_test.cc (right):

http://codereview.chromium.org/3171023/diff/4001/5009#newcode7
metrics_daemon_test.cc:7: #include <utime.h>
On 2010/08/24 06:13:29, petkov wrote:
> make this a separate top include (C headers).
> 

Done.

http://codereview.chromium.org/3171023/diff/4001/5009#newcode96
metrics_daemon_test.cc:96: mkdir(kTestDir, 0700);
On 2010/08/24 06:13:29, petkov wrote:
> Isn't there a file_util for creating a directory?
> 

Done.

http://codereview.chromium.org/3171023/diff/4001/5010
File metrics_library.cc (right):

http://codereview.chromium.org/3171023/diff/4001/5010#newcode154
metrics_library.cc:154: if (!AreMetricsEnabled())
On 2010/08/24 06:13:29, petkov wrote:
> You could move this inside SendMessageToChrome, and then you wouldn't need it
> both here and in SendEnumToUMA.
> 

Done.

http://codereview.chromium.org/3171023/diff/4001/5011
File metrics_library.h (right):

http://codereview.chromium.org/3171023/diff/4001/5011#newcode75
metrics_library.h:75: FRIEND_TEST(CMetricsLibraryTest, AreMetricsEnabled);
On 2010/08/24 06:13:29, petkov wrote:
> Do you need this friend?
> 

I need all the friends I can get.  But, no, I guess not this one.

[petkov, 2010-08-25 16:07:35]

metrics_daemon doesn't use epoch_start in this CL, right?

http://codereview.chromium.org/3171023/diff/13001/2005
File counter.h (right):

http://codereview.chromium.org/3171023/diff/13001/2005#newcode283
counter.h:283: static time_t epoch_start_;
Can you add a comment why this is static? Same for metrics_lib above.

Any chance different counters would need different epoch start?

http://codereview.chromium.org/3171023/diff/13001/2008
File metrics_daemon.cc (right):

http://codereview.chromium.org/3171023/diff/13001/2008#newcode243
metrics_daemon.cc:243:
sizeof(crash_interval_configs)/sizeof(crash_interval_configs[0]);
use arraysize?

http://codereview.chromium.org/3171023/diff/13001/2008#newcode285
metrics_daemon.cc:285:
sizeof(crash_frequency_configs)/sizeof(crash_frequency_configs[0]);
use arraysize?

http://codereview.chromium.org/3171023/diff/13001/2012
File metrics_library.h (right):

http://codereview.chromium.org/3171023/diff/13001/2012#newcode97
metrics_library.h:97: static time_t cached_enabled_time_;
Any reason for these to be static?

[kmixter1, 2010-08-26 18:52:31]

Using maps now to store all frequency counters which should simplify adding new
ones.  Also we now flush frequency counters whenever a cycle expires.  IOW, we
send a daily stat the moment we enter the next daily cycle, instead of waiting
until another countable occurrence happens.  This also enables us to send stats
of 0 occurrences.

http://codereview.chromium.org/3171023/diff/13001/2008
File metrics_daemon.cc (right):

http://codereview.chromium.org/3171023/diff/13001/2008#newcode243
metrics_daemon.cc:243:
sizeof(crash_interval_configs)/sizeof(crash_interval_configs[0]);
On 2010/08/25 16:07:35, petkov wrote:
> use arraysize?
> 

Reformatted to avoid this structs-inside-functions stuff.

http://codereview.chromium.org/3171023/diff/13001/2008#newcode285
metrics_daemon.cc:285:
sizeof(crash_frequency_configs)/sizeof(crash_frequency_configs[0]);
On 2010/08/25 16:07:35, petkov wrote:
> use arraysize?
> 

Reformatted to avoid this structs-inside-functions stuff.

http://codereview.chromium.org/3171023/diff/13001/2012
File metrics_library.h (right):

http://codereview.chromium.org/3171023/diff/13001/2012#newcode97
metrics_library.h:97: static time_t cached_enabled_time_;
On 2010/08/25 16:07:35, petkov wrote:
> Any reason for these to be static?
> 

It doesn't need to be, but I believe it's always better - we're looking up a
statically named file across all instances of MetricsLibrary, so caching across
all instances always seems better.

[petkov, 2010-08-26 20:46:14]

A few more questions/comments, LGTM otherwise.

Btw, it might have been good to split such changes into separate CLs -- for
example, the AreMetricsAllowed change can easily be separated (and could have
been checked in already)...

http://codereview.chromium.org/3171023/diff/19001/20004
File counter.h (right):

http://codereview.chromium.org/3171023/diff/19001/20004#newcode179
counter.h:179: // Initializes the counter by providing the persistent storage
Maybe specify somewheret that this creates/sends regular/exponential histograms
(rather then linear/enum).

http://codereview.chromium.org/3171023/diff/19001/20004#newcode182
counter.h:182: //
remove empty //

http://codereview.chromium.org/3171023/diff/19001/20007
File metrics_daemon.cc (right):

http://codereview.chromium.org/3171023/diff/19001/20007#newcode237
metrics_daemon.cc:237: scoped_ptr<chromeos_metrics::TaggedCounterReporter>
reporter(
scoped_ptr here seems unnecessary, right? given that you're releasing it to the
FrequencyCounter right away.

http://codereview.chromium.org/3171023/diff/19001/20007#newcode245
metrics_daemon.cc:245: scoped_ptr<chromeos_metrics::FrequencyCounter>
new_counter(
unnecessary scoped_ptr?

http://codereview.chromium.org/3171023/diff/19001/20007#newcode275
metrics_daemon.cc:275: frequency_counters_.clear();
How about moving this and the current destructor code into a separate
DeleteFrequencyCounters method and invoke that method both from here and from
the destructor?