Add the Stability.BrowserExitCodes metric and a couple of relevant exit codes.

Add the Stability.BrowserExitCodes metric (added in https://chromium.googlesource.com/chromium/src/+/ce04138298b8564e8027c98aee0b28a766ec76da) and a couple of relevant exit codes.

BUG=412384
Committed: https://crrev.com/646bd2dfd0ba8a8048aa730f29602ae1c1228fb6
Cr-Commit-Position: refs/heads/master@{#305327}

[Sigurður Ásgeirsson, 2014-11-21 20:35:44]

siggi@chromium.org changed reviewers:
+ erikwright@chromium.org, mpearson@chromium.org

[Sigurður Ásgeirsson, 2014-11-21 20:35:44]

Erik: For your review.
Mark: For owners approval, if you'd please?

[erikwright (departed), 2014-11-21 20:41:10]

LGTM.

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35135: +    Records the exit code of
browser process (on Windows) from the previous
of browser -> of the browser

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:56408: +<!-- TODO(siggi): Add other
values here at need. -->
indentation?

[Mark P, 2014-11-21 20:48:41]

Please link in the changelist description to the change that added this
histogram.

--mark

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35131: +<histogram
name="Stability.BrowserExitCodes" enum="WindowsExitCode">
Do you plan that this is always a Windows-only histogram?  If so, why did you
name the histogram as this?  If not, why are you naming the enum this?

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35136: +    launch. On crash, the exit
code will indicate the exception code of the
Please explain directly when this is emitted.  e.g., Emitted shortly after
startup on the following launch.

[Sigurður Ásgeirsson, 2014-11-21 21:59:31]

Thanks, another look?

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35131: +<histogram
name="Stability.BrowserExitCodes" enum="WindowsExitCode">
On 2014/11/21 20:48:41, Mark P wrote:
> Do you plan that this is always a Windows-only histogram?  If so, why did you
> name the histogram as this?  If not, why are you naming the enum this?

Yes, I expect this'll be Windows only. On sane OSen, we shouldn't need this, as
we can haz sane crash capture there.

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35135: +    Records the exit code of
browser process (on Windows) from the previous
On 2014/11/21 20:41:10, erikwright wrote:
> of browser -> of the browser

Done.

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:35136: +    launch. On crash, the exit
code will indicate the exception code of the
On 2014/11/21 20:48:41, Mark P wrote:
> Please explain directly when this is emitted.  e.g., Emitted shortly after
> startup on the following launch.

Done.

https://codereview.chromium.org/748193002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:56408: +<!-- TODO(siggi): Add other
values here at need. -->
On 2014/11/21 20:41:10, erikwright wrote:
> indentation?

The presubmit check for this file wants precisely this indentation.

[Mark P, 2014-11-21 22:48:51]

histograms.xml lgtm

The changelist description is wrong.  The histogram was added in
https://chromium.googlesource.com/chromium/src/+/ce04138298b8564e8027c98aee0b...
not in the change you pointed to there.

In the future, please include histograms.xml directly in the changelist where
you add the histogram.

Furthermore, I'd encourage you to revise your code to simply use the
UMA_HISTOGRAM_SPARSE_SLOWLY macro.  It'll save a couple of lines and also be
easier to read.

--mark

[Sigurður Ásgeirsson, 2014-11-21 23:16:33]

Thanks, CL description fixed.
Will in future endeavor to add to XML file with rest of change. Last time I did
this the XML file was in another repo :).
 
The precise variant of histogram I'm adding does not exist as a macro - note the
stability flag and where it's invoked. This will - from what I understand (I'm
often wrong) - ensure any crash-related exit codes are reported ASAP immediately
on launch with the stability report. This may in turn make it marginally easier
to reconcile histograms to crash data - one can hope.
Alexei indicated just to use the API directly in this case.

[Sigurður Ásgeirsson, 2014-11-21 23:16:41]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-11-21 23:17:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/748193002/20001

[Mark P, 2014-11-21 23:23:43]

On Fri, Nov 21, 2014 at 3:16 PM, <siggi@chromium.org> wrote:

> Thanks, CL description fixed.
> Will in future endeavor to add to XML file with rest of change. Last time
> I did
> this the XML file was in another repo :).
>
> The precise variant of histogram I'm adding does not exist as a macro -
> note the
> stability flag and where it's invoked. This will - from what I understand
> (I'm
> often wrong) - ensure any crash-related exit codes are reported ASAP
> immediately
> on launch with the stability report. This may in turn make it marginally
> easier
> to reconcile histograms to crash data - one can hope.
> Alexei indicated just to use the API directly in this case.
>

Ah, I didn't notice that special flag.  Sorry.  Alexei's advice is right.

>
> https://codereview.chromium.org/748193002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-11-22 00:19:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-22 00:21:07]

Patchset 2 (id:??) landed as
https://crrev.com/646bd2dfd0ba8a8048aa730f29602ae1c1228fb6
Cr-Commit-Position: refs/heads/master@{#305327}