Add support for collecting non-Chrome crash stats in Chrome OS

Add support for collecting non-Chrome crash stats in Chrome OS.

Extends the external_metrics interface to recognize a new metric kind (crash) which gives a string to describe which kind of crash.  We handle three currently non-Chrome user space crashes, kernel crashes, and unclean system shutdowns.  These are accumulated, but not yet put into xml uploads until the server side is ready.

BUG=chromium-os:9352
TEST=unit_tests and tested in chromeos that log messages were generated corresponding with crashes.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72154

[semenzato, 2011-01-06 17:06:33]

Generally seems OK, but I have a couple of questions.  Also, you'll need a
Chrome committer to LGTM it (I am not).

The post to the UI thread for external user actions is necessary because that's
where user actions are recorded.  Crashes are not user actions, neither do you
use (AFAICT) user-action-related calls, so it may be unnecessary and/or
incorrect to post to the UI thread.

The other question is, wouldn't it be better to change the server side first to
handle the new reports, so you don't have to stage another change?  Is there any
functional use in making this change first, other than it goes in the right
direction?  Does the server barf on reports it doesn't understand, or does it
handle them gracefully?

Regarding testing: you probably noticed the external metrics unit test I added. 
It's marginally useful and doesn't test that much, but I couldn't think of a
reasonably simple way of testing more extensively as it would involve too many
components.

[petkov, 2011-01-06 18:50:36]

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
File chrome/browser/chromeos/external_metrics.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.cc:94: NewRunnableMethod(this,
&ExternalMetrics::RecordCrashUI, crash_kind));
You probably want to stay on the file thread -- not sure...

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.cc:217: } else if (strcmp(name,
"crash") == 0) {
keep the ifs sorted?

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
File chrome/browser/chromeos/external_metrics.h (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.h:62: void RecordCrashUI(const
std::string &action_string);
I think they use */& to the left (std::string&) in Chrome. Also, rename the
argument.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_log.cc:235: count =
pref->GetInteger(prefs::kStabilityOtherUserCrashCount);
did you forget to add the file that declares/defines the prefs strings?

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_log.cc:239: LOG(WARNING) << "Would write
otherusercrashcount=" << count;
you'll remove these logs eventually, i assume.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.cc:1734:
HandleIdleSinceLastTransmission(false);
what does this do?

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.h:129: static std::string
GetHardwareClass();
I think this method declaration needs to be removed (there's no implementation
AFAICT) -- you may as well clean this up.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.h:139: #if defined(OS_CHROMEOS)
why don't you fold this into the ifdef above rather than introduce a new ifdef?

[kmixter1, 2011-01-14 00:07:01]

PTAL - responded to existing comments and added unit tests.

Adding Jim and Evan to the review now that I've responded to some of the more
basic problems.

I'm staging this change to demonstrate that various crash stats are coming up
from Chrome OS, added up and prepared for sending, but not yet being sent. 
tkirchner is looking into adding these on the server side so we can enable them.

One open question is if the crash logging code should be run on the UI thread
and if it needs to come through the MetricsService::Observe interface.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
File chrome/browser/chromeos/external_metrics.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.cc:94: NewRunnableMethod(this,
&ExternalMetrics::RecordCrashUI, crash_kind));
On 2011/01/06 18:50:36, petkov wrote:
> You probably want to stay on the file thread -- not sure...

I'm not sure either.  Will defer to an owner of this code as UI thread is where
all other processing happens in metrics_service and this routine doesn't block.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.cc:217: } else if (strcmp(name,
"crash") == 0) {
On 2011/01/06 18:50:36, petkov wrote:
> keep the ifs sorted?

Done.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
File chrome/browser/chromeos/external_metrics.h (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.h:62: void RecordCrashUI(const
std::string &action_string);
On 2011/01/06 18:50:36, petkov wrote:
> I think they use */& to the left (std::string&) in Chrome. Also, rename the
> argument.

Done.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_log.cc:235: count =
pref->GetInteger(prefs::kStabilityOtherUserCrashCount);
On 2011/01/06 18:50:36, petkov wrote:
> did you forget to add the file that declares/defines the prefs strings?

Done.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_log.cc:239: LOG(WARNING) << "Would write
otherusercrashcount=" << count;
On 2011/01/06 18:50:36, petkov wrote:
> you'll remove these logs eventually, i assume.

Yes, until the log server can handle these additional attributes (it throws away
tags with unexpected attributes) we just log them out locally to prove we're
capturing the right values.  Made intention clear in TODO.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.cc:1734:
HandleIdleSinceLastTransmission(false);
On 2011/01/06 18:50:36, petkov wrote:
> what does this do?

Fair question - see new comment.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.h:129: static std::string
GetHardwareClass();
On 2011/01/06 18:50:36, petkov wrote:
> I think this method declaration needs to be removed (there's no implementation
> AFAICT) -- you may as well clean this up.

Done.

http://codereview.chromium.org/6077013/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_service.h:139: #if defined(OS_CHROMEOS)
On 2011/01/06 18:50:36, petkov wrote:
> why don't you fold this into the ifdef above rather than introduce a new
ifdef?

Done.

[jar (doing other things), 2011-01-14 02:31:42]

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:112: void
MetricsLog::WriteStabilityElement(PrefService* pref) {
Why did you decide to pass this in?  Does this facilitate testing?

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:206:
pref->SetInteger(prefs::kStabilityKernelCrashCount, 11);
nit: four unique integers would probably be better (you repeated 11).

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:227:
ASSERT_TRUE(log.GetEncodedLog(WriteInto(&encoded, size + 1), size));
nit: I'd rather not see an ASSERT with side effects.  Better IMO is assert a
return value, but have the action outside the assert.  (you could argue these
are test-harness asserts... but I think readability is better when you can see
action code, and then look at tested assertions separately).

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:231: // servers can handle them.
:-) Nice!

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1727:
IncrementPrefValue(prefs::kStabilityOtherUserCrashCount);
Is this all on the UI thread?

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (left):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:134: static std::string
GetHardwareClass();
Why is this mixed in with this CL?

[kmixter1, 2011-01-15 22:21:07]

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:112: void
MetricsLog::WriteStabilityElement(PrefService* pref) {
On 2011/01/14 02:31:47, jar wrote:
> Why did you decide to pass this in?  Does this facilitate testing?

Exactly.  So I can unit test what this function outputs specifically in
metrics_log_unittest, based on a mocked PrefService.

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:206:
pref->SetInteger(prefs::kStabilityKernelCrashCount, 11);
On 2011/01/14 02:31:47, jar wrote:
> nit: four unique integers would probably be better (you repeated 11).

I didn't even notice that - 4 different was my intention. Done.

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:227:
ASSERT_TRUE(log.GetEncodedLog(WriteInto(&encoded, size + 1), size));
On 2011/01/14 02:31:47, jar wrote:
> nit: I'd rather not see an ASSERT with side effects.  Better IMO is assert a
> return value, but have the action outside the assert.  (you could argue these
> are test-harness asserts... but I think readability is better when you can see
> action code, and then look at tested assertions separately).

Done.

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:231: // servers can handle them.
On 2011/01/14 02:31:47, jar wrote:
> :-) Nice!

Done.

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1727:
IncrementPrefValue(prefs::kStabilityOtherUserCrashCount);
On 2011/01/14 02:31:47, jar wrote:
> Is this all on the UI thread?

In external_metrics I only call this when on the UI thread.  Are there any
common conventions I should be following (like asserting the thread here)?  And
is the UI thread the appropriate one?

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (left):

http://codereview.chromium.org/6077013/diff/28001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:134: static std::string
GetHardwareClass();
On 2011/01/14 02:31:47, jar wrote:
> Why is this mixed in with this CL?

It's completely unrelated.  Darin noticed this function was not implemented and
I removed it as part of this CL.  Could split it out if you'd rather.

[jar (doing other things), 2011-01-16 07:14:33]

It has historically been more common (simpler?) to first change the log server
to accept new data, and then to change the client to send the data.  This
landing is something of a partial change, that almost enables the client. Given
that you'll need to enable the client in the future (with another CL?), you
could just wait and land the whole thing.  That said, I don't mind if you do it
in two steps.... but it is your choice.

LGTM  (but you could wait for the time that you can fully enable this before
landing).

[jar (doing other things), 2011-01-18 22:32:22]

Ken asked about the UI thread post.... so I added a comment...

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
File chrome/browser/chromeos/external_metrics.cc (right):

http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
chrome/browser/chromeos/external_metrics.cc:94: NewRunnableMethod(this,
&ExternalMetrics::RecordCrashUI, crash_kind));
I believe jumping over to the UI thread is both correct and critical.  Without
this PostTask, values will be read (for upload) on the UI thread, and altered
here (on another thread?).


On 2011/01/14 00:07:02, kmixter1 wrote:
> On 2011/01/06 18:50:36, petkov wrote:
> > You probably want to stay on the file thread -- not sure...
> 
> I'm not sure either.  Will defer to an owner of this code as UI thread is
where
> all other processing happens in metrics_service and this routine doesn't
block.

[kmixter1, 2011-01-20 23:54:10]

I agree it would be simpler to block on the server implementation, but its
timing isn't clear yet, so I'd like to checkpoint this along with the Chrome
OS changes that depend on it.

With Jim's LGTM, Darin, could you land this?


On Tue, Jan 18, 2011 at 2:32 PM, <jar@chromium.org> wrote:

> Ken asked about the UI thread post.... so I added a comment...
>
>
>
>
>
http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
> File chrome/browser/chromeos/external_metrics.cc (right):
>
>
>
http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
> chrome/browser/chromeos/external_metrics.cc:94: NewRunnableMethod(this,
> &ExternalMetrics::RecordCrashUI, crash_kind));
> I believe jumping over to the UI thread is both correct and critical.
> Without this PostTask, values will be read (for upload) on the UI
> thread, and altered here (on another thread?).
>
>
>
> On 2011/01/14 00:07:02, kmixter1 wrote:
>
>> On 2011/01/06 18:50:36, petkov wrote:
>> > You probably want to stay on the file thread -- not sure...
>>
>
>  I'm not sure either.  Will defer to an owner of this code as UI thread
>>
> is where
>
>> all other processing happens in metrics_service and this routine
>>
> doesn't block.
>
> http://codereview.chromium.org/6077013/
>

[petkov, 2011-01-21 18:53:30]

On 2011/01/20 23:54:10, kmixter1 wrote:
> I agree it would be simpler to block on the server implementation, but its
> timing isn't clear yet, so I'd like to checkpoint this along with the Chrome
> OS changes that depend on it.
> 
> With Jim's LGTM, Darin, could you land this?

Done.

> 
> 
> On Tue, Jan 18, 2011 at 2:32 PM, <mailto:jar@chromium.org> wrote:
> 
> > Ken asked about the UI thread post.... so I added a comment...
> >
> >
> >
> >
> >
>
http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
> > File chrome/browser/chromeos/external_metrics.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6077013/diff/1/chrome/browser/chromeos/externa...
> > chrome/browser/chromeos/external_metrics.cc:94: NewRunnableMethod(this,
> > &ExternalMetrics::RecordCrashUI, crash_kind));
> > I believe jumping over to the UI thread is both correct and critical.
> > Without this PostTask, values will be read (for upload) on the UI
> > thread, and altered here (on another thread?).
> >
> >
> >
> > On 2011/01/14 00:07:02, kmixter1 wrote:
> >
> >> On 2011/01/06 18:50:36, petkov wrote:
> >> > You probably want to stay on the file thread -- not sure...
> >>
> >
> >  I'm not sure either.  Will defer to an owner of this code as UI thread
> >>
> > is where
> >
> >> all other processing happens in metrics_service and this routine
> >>
> > doesn't block.
> >
> > http://codereview.chromium.org/6077013/
> >