Switch stability reports to use the crashed version's details

Switch stability reports to use the crashed version's details

Prior to this CL we report the reporter's version details. This CL
switches to using the crasher's version. The reporter's version is
added to the report as global data.

There may be cases in which the crasher's version details are missing.
This CL assumes this is rare. In such cases, no report is sent but we log
to ActivityTracker.Collect.MissingProductDetails.

BUG=687715
Review-Url: https://codereview.chromium.org/2685053003
Cr-Commit-Position: refs/heads/master@{#450367}
Committed: https://chromium.googlesource.com/chromium/src/+/6b9a40a37854fe18d08e55421d05f69973a6038d

[manzagop (departed), 2017-02-10 19:44:11]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 19:45:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-02-10 19:49:08]

manzagop@chromium.org changed reviewers:
+ bcwhite@chromium.org

[manzagop (departed), 2017-02-10 19:49:33]

Hi Brian!
Could you have a look?
Thanks!
Pierre

[commit-bot: I haz the power, 2017-02-10 20:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 20:31:11]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[manzagop (departed), 2017-02-10 21:24:13]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 21:24:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 22:42:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 22:42:19]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-02-13 13:16:59]

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:71: return false;
Is it possible to fetch only partial data?  Would that be a "failure"?

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win_unittest.cc
(right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:58:
google::protobuf::Map<std::string, TypedValue>* global_data =
You can make this a reference type and dereference the the returned pointer
here, thus avoiding (*global_data) everywhere below.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.cc (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:289:
google::protobuf::Map<std::string, TypedValue>* global_data =
You can change this to a reference type and then dereference the pointer here
and so avoid (*global_data) everywhere below.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:119: // Tag id 10 is reserved.
Place after #9.  What is it reserved for?

https://codereview.chromium.org/2685053003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2685053003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:374: +<histogram
name="ActivityTracker.Collect.MissingProductDetails" units="count">
Do you want to generalize this to something like AT.Collect.Result with
"success" and "missingdetails" as enum values?  That would allow it to be
extended to include other failure cases.

[manzagop (departed), 2017-02-13 17:07:45]

Thanks! Please have another look.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:71: return false;
On 2017/02/13 13:16:59, bcwhite wrote:
> Is it possible to fetch only partial data?  Would that be a "failure"?

Missing fields are unlikely (all 4 are recorded as soon as we enable tracking)
but still possible. I'm thinking we can start by considering it a failure
because it's likely not worth the effort of handling unless the UMA metric says
otherwise.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win_unittest.cc
(right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:58:
google::protobuf::Map<std::string, TypedValue>* global_data =
On 2017/02/13 13:16:59, bcwhite wrote:
> You can make this a reference type and dereference the the returned pointer
> here, thus avoiding (*global_data) everywhere below.

Done.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.cc (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:289:
google::protobuf::Map<std::string, TypedValue>* global_data =
On 2017/02/13 13:16:59, bcwhite wrote:
> You can change this to a reference type and then dereference the pointer here
> and so avoid (*global_data) everywhere below.

Done.

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:119: // Tag id 10 is reserved.
On 2017/02/13 13:16:59, bcwhite wrote:
> Place after #9.  What is it reserved for?

It's for augmenting the report with symbolized info (populated on the server).

https://codereview.chromium.org/2685053003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2685053003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:374: +<histogram
name="ActivityTracker.Collect.MissingProductDetails" units="count">
On 2017/02/13 13:16:59, bcwhite wrote:
> Do you want to generalize this to something like AT.Collect.Result with
> "success" and "missingdetails" as enum values?  That would allow it to be
> extended to include other failure cases.

Done.

[manzagop (departed), 2017-02-13 17:11:58]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 17:12:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 18:24:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 18:24:17]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-02-13 18:35:07]

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:119: // Tag id 10 is reserved.
On 2017/02/13 17:07:45, manzagop wrote:
> On 2017/02/13 13:16:59, bcwhite wrote:
> > Place after #9.  What is it reserved for?
> 
> It's for augmenting the report with symbolized info (populated on the server).

Please add that to the comment.  And move it after #9 below?

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:105: NONE = 0,
Omit NONE if you never send it.

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:230:
UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.WriteDumpStatus", FAILED,
To avoid duplicate histogram instantiation code, rearrange this so into "if
(success) { ... }" code and have a common "status" variable that gets set
appropriately.  Then update the histogram once below.

Alternatively, you can create a separate method (UpdateDumpStatusHistogram?) and
call that with the desired code.

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:246: WriteStatus
status = success ? SUCCESS : FAILED;
Move this inline below.

[manzagop (departed), 2017-02-13 19:21:37]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[manzagop (departed), 2017-02-13 19:22:36]

Thanks! Another look?

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:119: // Tag id 10 is reserved.
On 2017/02/13 18:35:07, bcwhite wrote:
> On 2017/02/13 17:07:45, manzagop wrote:
> > On 2017/02/13 13:16:59, bcwhite wrote:
> > > Place after #9.  What is it reserved for?
> > 
> > It's for augmenting the report with symbolized info (populated on the
server).
> 
> Please add that to the comment.  And move it after #9 below?

Expanded on the comment, but purposefully remaining vague.

Keeping the location, as it's a better semantically. I think this is pretty
common for protocol buffers (and the reason for folks using the "Next id"
comment (as you can't rely on the bottom most id)).

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:105: NONE = 0,
On 2017/02/13 18:35:07, bcwhite wrote:
> Omit NONE if you never send it.

Done.

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:230:
UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.WriteDumpStatus", FAILED,
On 2017/02/13 18:35:07, bcwhite wrote:
> To avoid duplicate histogram instantiation code, rearrange this so into "if
> (success) { ... }" code and have a common "status" variable that gets set
> appropriately.  Then update the histogram once below.
> 
> Alternatively, you can create a separate method (UpdateDumpStatusHistogram?)
and
> call that with the desired code.

Ah right! Went with the latter, to limit indentations.

https://codereview.chromium.org/2685053003/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:246: WriteStatus
status = success ? SUCCESS : FAILED;
On 2017/02/13 18:35:07, bcwhite wrote:
> Move this inline below.

Done.

[commit-bot: I haz the power, 2017-02-13 19:22:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-02-13 20:08:22]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 20:09:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 22:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 22:05:20]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-02-14 15:18:16]

lgtm

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2685053003/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:119: // Tag id 10 is reserved.
On 2017/02/13 19:22:33, manzagop wrote:
> On 2017/02/13 18:35:07, bcwhite wrote:
> > On 2017/02/13 17:07:45, manzagop wrote:
> > > On 2017/02/13 13:16:59, bcwhite wrote:
> > > > Place after #9.  What is it reserved for?
> > > 
> > > It's for augmenting the report with symbolized info (populated on the
> server).
> > 
> > Please add that to the comment.  And move it after #9 below?
> 
> Expanded on the comment, but purposefully remaining vague.
> 
> Keeping the location, as it's a better semantically. I think this is pretty
> common for protocol buffers (and the reason for folks using the "Next id"
> comment (as you can't rely on the bottom most id)).

Acknowledged.

[manzagop (departed), 2017-02-14 15:26:30]

Thanks!

[manzagop (departed), 2017-02-14 15:27:11]

manzagop@chromium.org changed reviewers:
+ rkaplow@chromium.org

[manzagop (departed), 2017-02-14 15:27:12]

Hi Rob!

Could you have an OWNERS' look at:
  tools/metrics/histograms/histograms.xml

Thanks!

Pierre

[rkaplow, 2017-02-14 15:29:49]

lgtm

histogram lg

[manzagop (departed), 2017-02-14 15:46:33]

Thanks!

[manzagop (departed), 2017-02-14 15:46:43]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2017-02-14 15:47:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 15:51:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487087203056670,
"parent_rev": "226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d", "commit_rev":
"6b9a40a37854fe18d08e55421d05f69973a6038d"}

[commit-bot: I haz the power, 2017-02-14 15:52:20]

Description was changed from

==========
Switch stability reports to use the crashed version's details

Prior to this CL we report the reporter's version details. This CL
switches to using the crasher's version. The reporter's version is
added to the report as global data.

There may be cases in which the crasher's version details are missing.
This CL assumes this is rare. In such cases, no report is sent but we log
to ActivityTracker.Collect.MissingProductDetails.

BUG=687715
==========

to

==========
Switch stability reports to use the crashed version's details

Prior to this CL we report the reporter's version details. This CL
switches to using the crasher's version. The reporter's version is
added to the report as global data.

There may be cases in which the crasher's version details are missing.
This CL assumes this is rare. In such cases, no report is sent but we log
to ActivityTracker.Collect.MissingProductDetails.

BUG=687715

Review-Url: https://codereview.chromium.org/2685053003
Cr-Commit-Position: refs/heads/master@{#450367}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b9a40a37854fe18d08e55421d05...
==========

[commit-bot: I haz the power, 2017-02-14 15:52:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6b9a40a37854fe18d08e55421d05...