Collect log messages from stability files

Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the process level. We may want to revisit that if/when reports start containing multiple processes

BUG=620813
Committed: https://crrev.com/c3893cf956336e8e50ede22da689f07cb48a7e89
Cr-Commit-Position: refs/heads/master@{#438208}

[manzagop (departed), 2016-12-07 19:20:35]

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

[manzagop (departed), 2016-12-07 19:20:49]

Please have a look!

[manzagop (departed), 2016-12-07 19:22:04]

On 2016/12/07 19:20:49, manzagop wrote:
> Please have a look!

(Btw, the exclamation mark is to signal enthusiasm, and not an indication of
hurry. :)

[manzagop (departed), 2016-12-07 19:33:07]

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

[commit-bot: I haz the power, 2016-12-07 19:33:47]

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

[manzagop (departed), 2016-12-07 19:34:42]

Description was changed from

==========
Collect log messages from stability files

Collection was added in https://codereview.chromium.org/2544853003/

BUG=620813
==========

to

==========
Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the
process level. We may want to revisit that if/when reports start containing
multiple processes

BUG=620813
==========

[commit-bot: I haz the power, 2016-12-07 20:55:04]

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

[commit-bot: I haz the power, 2016-12-07 20:55:05]

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

[manzagop (departed), 2016-12-07 21:50:17]

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

[commit-bot: I haz the power, 2016-12-07 21:50:53]

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, 2016-12-07 22:36:25]

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

[commit-bot: I haz the power, 2016-12-07 22:36:26]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-09 00:34:22]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
It's possible to have log messages even on a clean exit.  Right now it's limited
to FATAL messages but that's not part of the definition.

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:136: repeated string
log_messages = 3;
I think this should not be plural given the way it's used:  add_log_message,
log_message_count, etc.  'course, that would apply to the other repeated fields,
too.

[manzagop (departed), 2016-12-09 22:41:10]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
On 2016/12/09 00:34:22, bcwhite wrote:
> It's possible to have log messages even on a clean exit.  Right now it's
limited
> to FATAL messages but that's not part of the definition.

Right: if we stop deleting the file on clean exit, then we'll need to add some
logic to know if we should send a report (ie if the exit was clean or not).

Do you feel a change is needed?

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/stability_report.proto:136: repeated string
log_messages = 3;
On 2016/12/09 00:34:22, bcwhite wrote:
> I think this should not be plural given the way it's used:  add_log_message,
> log_message_count, etc.  'course, that would apply to the other repeated
fields,
> too.

Plural is what the style guide says (not chromium's) but +1 to function name
being so-so.

[bcwhite, 2016-12-09 23:17:06]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
On 2016/12/09 22:41:09, manzagop wrote:
> On 2016/12/09 00:34:22, bcwhite wrote:
> > It's possible to have log messages even on a clean exit.  Right now it's
> limited
> > to FATAL messages but that's not part of the definition.
> 
> Right: if we stop deleting the file on clean exit, then we'll need to add some
> logic to know if we should send a report (ie if the exit was clean or not).
> 
> Do you feel a change is needed?

log_messages being empty or non-empty doesn't consistently indicate crash-exit
or clean-exit.

I don't think you should check it at all when deciding whether it was a clean
exit.

[manzagop (departed), 2016-12-12 15:59:18]

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

[manzagop (departed), 2016-12-12 15:59:33]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
On 2016/12/09 23:17:06, bcwhite wrote:
> On 2016/12/09 22:41:09, manzagop wrote:
> > On 2016/12/09 00:34:22, bcwhite wrote:
> > > It's possible to have log messages even on a clean exit.  Right now it's
> > limited
> > > to FATAL messages but that's not part of the definition.
> > 
> > Right: if we stop deleting the file on clean exit, then we'll need to add
some
> > logic to know if we should send a report (ie if the exit was clean or not).
> > 
> > Do you feel a change is needed?
> 
> log_messages being empty or non-empty doesn't consistently indicate crash-exit
> or clean-exit.
> 
> I don't think you should check it at all when deciding whether it was a clean
> exit.

I've removed the mention of clean exit: this should only be about whether the
file is empty. 

Our current decision of clean vs not is whether the file exists, since we open
it for deletion on clean exit. You're right that we *also* don't send a report
if it's fully empty.

I've also updated the comment where this function is called to state the current
behavior: existing file that is non-empty = send a report.

[commit-bot: I haz the power, 2016-12-12 15:59: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, 2016-12-12 16:55:09]

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

[commit-bot: I haz the power, 2016-12-12 16:55:10]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-13 13:22:55]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
On 2016/12/12 15:59:32, manzagop wrote:
> On 2016/12/09 23:17:06, bcwhite wrote:
> > On 2016/12/09 22:41:09, manzagop wrote:
> > > On 2016/12/09 00:34:22, bcwhite wrote:
> > > > It's possible to have log messages even on a clean exit.  Right now it's
> > > limited
> > > > to FATAL messages but that's not part of the definition.
> > > 
> > > Right: if we stop deleting the file on clean exit, then we'll need to add
> some
> > > logic to know if we should send a report (ie if the exit was clean or
not).
> > > 
> > > Do you feel a change is needed?
> > 
> > log_messages being empty or non-empty doesn't consistently indicate
crash-exit
> > or clean-exit.
> > 
> > I don't think you should check it at all when deciding whether it was a
clean
> > exit.
> 
> I've removed the mention of clean exit: this should only be about whether the
> file is empty. 
> 
> Our current decision of clean vs not is whether the file exists, since we open
> it for deletion on clean exit. You're right that we *also* don't send a report
> if it's fully empty.
> 
> I've also updated the comment where this function is called to state the
current
> behavior: existing file that is non-empty = send a report.

So to be clear...  You _do_ want to send a report if _any_ log messages were
collected, keeping in mind that these messages may not be "fatal" messages.

Though this is currently the case but may not be so in the future which would
mean, should it change, that a report would get sent every time.

[manzagop (departed), 2016-12-13 14:01:39]

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

https://codereview.chromium.org/2554423002/diff/40001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:172: if
(log_messages.empty() && !thread_analyzer) {
On 2016/12/13 13:22:55, bcwhite wrote:
> On 2016/12/12 15:59:32, manzagop wrote:
> > On 2016/12/09 23:17:06, bcwhite wrote:
> > > On 2016/12/09 22:41:09, manzagop wrote:
> > > > On 2016/12/09 00:34:22, bcwhite wrote:
> > > > > It's possible to have log messages even on a clean exit.  Right now
it's
> > > > limited
> > > > > to FATAL messages but that's not part of the definition.
> > > > 
> > > > Right: if we stop deleting the file on clean exit, then we'll need to
add
> > some
> > > > logic to know if we should send a report (ie if the exit was clean or
> not).
> > > > 
> > > > Do you feel a change is needed?
> > > 
> > > log_messages being empty or non-empty doesn't consistently indicate
> crash-exit
> > > or clean-exit.
> > > 
> > > I don't think you should check it at all when deciding whether it was a
> clean
> > > exit.
> > 
> > I've removed the mention of clean exit: this should only be about whether
the
> > file is empty. 
> > 
> > Our current decision of clean vs not is whether the file exists, since we
open
> > it for deletion on clean exit. You're right that we *also* don't send a
report
> > if it's fully empty.
> > 
> > I've also updated the comment where this function is called to state the
> current
> > behavior: existing file that is non-empty = send a report.
> 
> So to be clear...  You _do_ want to send a report if _any_ log messages were
> collected, keeping in mind that these messages may not be "fatal" messages.
> 
> Though this is currently the case but may not be so in the future which would
> mean, should it change, that a report would get sent every time.

That's right: short term we rely on MarkStabilityFileForDeletion to know if the
exit was unclean:
https://cs.chromium.org/chromium/src/components/browser_watcher/stability_deb...

Longer term we'll get rid of MarkStabilityFileForDeletion and cook up some kind
of analysis for which presence of logs won't be an indicator of unclean
shutdown.

As we found out, having thread activity data also isn't an indicator of unclean
shutdown.

[bcwhite, 2016-12-13 15:53:15]

lgtm

[manzagop (departed), 2016-12-13 16:21:14]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2016-12-13 16:21:38]

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, 2016-12-13 17:36:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481646074295840,
"parent_rev": "b8faa5b53eeade2f5f17745ed3af4af4bcc4cdee", "commit_rev":
"ac6ca0445071c13377d2afc314918bb064a9e2dc"}

[commit-bot: I haz the power, 2016-12-13 17:36:45]

Description was changed from

==========
Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the
process level. We may want to revisit that if/when reports start containing
multiple processes

BUG=620813
==========

to

==========
Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the
process level. We may want to revisit that if/when reports start containing
multiple processes

BUG=620813

Review-Url: https://codereview.chromium.org/2554423002
==========

[commit-bot: I haz the power, 2016-12-13 17:36:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-13 17:40:15]

Description was changed from

==========
Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the
process level. We may want to revisit that if/when reports start containing
multiple processes

BUG=620813

Review-Url: https://codereview.chromium.org/2554423002
==========

to

==========
Collect log messages from stability files

Details:
- Collection was added in https://codereview.chromium.org/2544853003/
- log messages are added to the stability report's top level and not to the
process level. We may want to revisit that if/when reports start containing
multiple processes

BUG=620813

Committed: https://crrev.com/c3893cf956336e8e50ede22da689f07cb48a7e89
Cr-Commit-Position: refs/heads/master@{#438208}
==========

[commit-bot: I haz the power, 2016-12-13 17:40:16]

Patchset 4 (id:??) landed as
https://crrev.com/c3893cf956336e8e50ede22da689f07cb48a7e89
Cr-Commit-Position: refs/heads/master@{#438208}