Store gpu crashes in the systemprofileproto

Store gpu crashes in the systemprofileproto as well as in the histogram.

BUG=323250

[rkaplow, 2013-12-13 22:20:30]

Getting comments from you before sending to GPU owners.

[Alexei Svitkine (slow), 2013-12-13 22:37:22]

You should create a chromium bug and set it on the review via BUG=.

Also, perhaps a test could be added for this?

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:757: case
content::NOTIFICATION_GPU_PROCESS_CLOSED: {
Nit: blank line before this; also {}'s unnecessary.

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:252: // Call when there is a crash in
the GPU process.
Nit: "Records a GPU process crash."

Any reason this is protected and not in the private section below?

https://codereview.chromium.org/108683003/diff/40001/chrome/common/metrics/pr...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:402: // All types of GPU
crashes. Further details can be seen in the
Nit: Use same wording as other comments... "The number of times ..."

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1512: 
Nit: Remove extra blank line.

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.h:487: 
Nit: Remove extra blank line.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:112: void
GpuProcessHostUIShim::Destroy(int host_id, base::TerminationStatus status,
Nit: 1 param per line.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:118:
content::NOTIFICATION_GPU_PROCESS_CLOSED,
Nit: Indent 2 more, here and the lines below.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.h:58: static void Destroy(int
host_id, base::TerminationStatus status,
Nit: 1 param per line and align the params.

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
File content/public/browser/notification_types.h (right):

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
content/public/browser/notification_types.h:169: // Indicated that a GPU process
was closed and it had terminated abnormally.
Nit: Indicates

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
content/public/browser/notification_types.h:170:
NOTIFICATION_GPU_PROCESS_CLOSED,
How about CLOSED_ABNORMALLY?

[rkaplow, 2013-12-13 23:54:03]

Should also mention that I've already gotten a cl LGTM from log team for server
side log change.

I've added a small test for loading the crash count from prefs. Another test
that would make sense is to verify the notification sent from the
gpu_process_host_ui_shim which I have not done yet, I wouldn't mind verifying
that they are happy with using notifications first though.

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:757: case
content::NOTIFICATION_GPU_PROCESS_CLOSED: {
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: blank line before this; also {}'s unnecessary.

ok, was following most of the style from this method actually but the short ones
don't have so makes sense.

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:252: // Call when there is a crash in
the GPU process.
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: "Records a GPU process crash."
> 
> Any reason this is protected and not in the private section below?

ah no, remnant from a previous version

https://codereview.chromium.org/108683003/diff/40001/chrome/common/metrics/pr...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:402: // All types of GPU
crashes. Further details can be seen in the
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: Use same wording as other comments... "The number of times ..."

Done.

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1512: 
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/108683003/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.h:487: 
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:112: void
GpuProcessHostUIShim::Destroy(int host_id, base::TerminationStatus status,
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: 1 param per line.

Done.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:118:
content::NOTIFICATION_GPU_PROCESS_CLOSED,
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: Indent 2 more, here and the lines below.

Done.

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/108683003/diff/40001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.h:58: static void Destroy(int
host_id, base::TerminationStatus status,
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: 1 param per line and align the params.

done, aligned on another method as well.

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
File content/public/browser/notification_types.h (right):

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
content/public/browser/notification_types.h:169: // Indicated that a GPU process
was closed and it had terminated abnormally.
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> Nit: Indicates

Done.

https://codereview.chromium.org/108683003/diff/40001/content/public/browser/n...
content/public/browser/notification_types.h:170:
NOTIFICATION_GPU_PROCESS_CLOSED,
On 2013/12/13 22:37:22, Alexei Svitkine wrote:
> How about CLOSED_ABNORMALLY?

Done.

[Alexei Svitkine (slow), 2013-12-16 16:04:27]

LGTM with a few more nits.

https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:460: // Call when there is a crash in
the GPU process.
Nit: "Records a GPU process crash."

I'd also move this method next to the other Log*() methods above (and use the
same order in the .cc file).

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:116: // is used to signal a gpu
crash in the metrics service.
Nit: Avoid "we". How about:

"Send a notification if the GPU process did not shut down normally. This is used
to signal a GPU crash to the metrics service."

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.h:57: // UI shim. We also pass in
the status of the GPU termination.
Nit: Describe the params without using "we" (and describe |message| while you're
here.)

[rkaplow, 2013-12-16 16:39:53]

https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:460: // Call when there is a crash in
the GPU process.
On 2013/12/16 16:04:28, Alexei Svitkine wrote:
> Nit: "Records a GPU process crash."
> 
> I'd also move this method next to the other Log*() methods above (and use the
> same order in the .cc file).

This was sort of intentional since there is no real ordering (i.e. there is
LogLoadStarted and a bunch of Log* methods at line ~230. I figured I would just
put it last. At any rate, I don't mind putting it with those, but the file needs
to be reordered anyways.

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:116: // is used to signal a gpu
crash in the metrics service.
On 2013/12/16 16:04:28, Alexei Svitkine wrote:
> Nit: Avoid "we". How about:
> 
> "Send a notification if the GPU process did not shut down normally. This is
used
> to signal a GPU crash to the metrics service."

Done.

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/108683003/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.h:57: // UI shim. We also pass in
the status of the GPU termination.
On 2013/12/16 16:04:28, Alexei Svitkine wrote:
> Nit: Describe the params without using "we" (and describe |message| while
you're
> here.)

good point, thanks

[rkaplow, 2013-12-16 16:51:59]

If I could get this reviewed for ownership that would be great.

[piman, 2013-12-16 22:12:04]

I'm not sure what's the idea behind routing this to the metrics_service, when we
already have the UMA histograms.

But either way, I'm hesitant to introduce more Notifications, which are known to
be problematic. Could we do it through an Observer pattern, or a Callback?

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host.cc:32: #include
"content/public/browser/notification_types.h"
Those 2 headers don't look used. Remove?

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:118:
content::NotificationService::current()->Notify(
We're moving away from the Notification pattern, at least in content/ (see
discussions in chromium-dev). Can this be done with an Observer pattern, or by
registering a Callback?

[Ilya Sherman, 2013-12-16 23:04:39]

https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:404: optional int32
gpu_crash_count = 23;
nit: Hmm, I would probably move this to be grouped with the other crash
counters, or else to be after item 22, if you're keen on trying to keep the list
in order.

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:118:
content::NotificationService::current()->Notify(
On 2013/12/16 22:12:04, piman wrote:
> We're moving away from the Notification pattern, at least in content/ (see
> discussions in chromium-dev). Can this be done with an Observer pattern, or by
> registering a Callback?

+1

[rkaplow, 2013-12-18 16:28:42]

On 2013/12/16 23:04:39, Ilya Sherman wrote:
>
https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
> chrome/common/metrics/proto/system_profile.proto:404: optional int32
> gpu_crash_count = 23;
> nit: Hmm, I would probably move this to be grouped with the other crash
> counters, or else to be after item 22, if you're keen on trying to keep the
list
> in order.
> 
>
https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
> File content/browser/gpu/gpu_process_host_ui_shim.cc (right):
> 
>
https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
> content/browser/gpu/gpu_process_host_ui_shim.cc:118:
> content::NotificationService::current()->Notify(
> On 2013/12/16 22:12:04, piman wrote:
> > We're moving away from the Notification pattern, at least in content/ (see
> > discussions in chromium-dev). Can this be done with an Observer pattern, or
by
> > registering a Callback?
> 
> +1


Regarding piman's comment for why this is required. The original reason I was
working on this is I am changing some of the tools server side that would want
all the stability crash metrics in the same place. However after thinking about
it there is a pretty straightforward workaround - i.e. I can still get it from
the current histogram location, and just handle it a bit server side.

In that case the main reason for it is probably consistency, is it a type of
chrome crash and that analysts might expect for it to reside in this location.
It would make it easier for someone querying the logs direct to find the gpu
crashes if they were here.

However for my purposes it actually isn't as required as I thought and it's
actually not such a huge deal to me. Do people think the consistency of having
it also there is counterbalanced by the additional logging / code complexity?
I'm not against dropping if if people don't think it is worth it.