Decouple stability instrumentation recording and collection

Decouple stability instrumentation recording and collection

Introduces an experiment parameter to control postmortem collection.
Depending on the parameter's value, stability files are either
collected or simply deleted.

Additional details:
- UncleanShutdownCount and UncleanSystemCount metrics are deprecated
in favor of additional events in ActivityTracker.Collect.Status. 
This is more memory efficient.

BUG=691595
Review-Url: https://codereview.chromium.org/2923523002
Cr-Commit-Position: refs/heads/master@{#477085}
Committed: https://chromium.googlesource.com/chromium/src/+/194acde6446e88d5ac045e9768ef53aea4319def

[manzagop (departed), 2017-06-05 14:13:26]

Patchset #1 (id:1) has been deleted

[manzagop (departed), 2017-06-05 14:18:14]

Description was changed from

==========
Decouple stability instrumentation recording from collection

BUG=
==========

to

==========
Decouple stability instrumentation recording and collection

Introduces an experiment parameter to control postmortem collection.
Depending on the parameter's value, stability files are either
collected or simply deleted.

Additional details:
- UncleanShutdownCount and UncleanSystemCount metrics are deprecated
in favor of additional events in ActivityTracker.Collect.Status. 
This is more memory efficient.

BUG=691595
==========

[manzagop (departed), 2017-06-05 14:33:03]

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

[commit-bot: I haz the power, 2017-06-05 14:33:20]

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-06-05 15:06:06]

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

[commit-bot: I haz the power, 2017-06-05 15:06:21]

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-06-05 15:06:29]

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

[manzagop (departed), 2017-06-05 15:06:30]

Hi Siggi,

could you have a look?

Thanks!

P-A

[Sigurður Ásgeirsson, 2017-06-05 15:39:39]

nice - have some comments.

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

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:65:
base::DeleteFile(file, false)
I'm not fond of using the trinary operator in this manner. Since you're after
the side-effects of the branches, and not their value, just write this as an if
statement.

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:132:
LogCollectionStatus(DEBUG_FILE_DELETION_FAILED);
Out of curiosity - assuming a file is somehow permanently stuck (like marked
read-only, or has an impenetrable ACL) it would accrue counts here for
perpetuity?
I'm not saying that's a problem, just wondering.

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.h (right):

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.h:32: class
PostmortemProcessor {
I don't see that hoisting an interface here is helping you in any way, shape or
form. I don't see any case where you're using polymorphic dispatch to these
classes - it's always instantiate one or the other, then call it.

I'd say just remove this interface have two unrelated classes handling the two
cases, unless I've missed something?

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

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector_unittest.cc:93:
DEBUG_FILE_DELETION_FAILED, 1);
nice. Stupid question, though - wouldn't the Postmortem reporter class satisfy
this test as-written just fine? Maybe add a negative test to make sure it's not
updating the counts it shouldn't?

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
File components/browser_watcher/stability_paths.h (right):

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/stability_paths.h:42: // excluding those in
|excluded_stability_files.
nit: closing bar missing.

[commit-bot: I haz the power, 2017-06-05 16:44:53]

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

[commit-bot: I haz the power, 2017-06-05 16:44:54]

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), 2017-06-05 18:01:46]

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

[commit-bot: I haz the power, 2017-06-05 18:02:12]

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-06-05 18:02:16]

Thanks! Another look?

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

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:65:
base::DeleteFile(file, false)
On 2017/06/05 15:39:39, Sigurður Ásgeirsson wrote:
> I'm not fond of using the trinary operator in this manner. Since you're after
> the side-effects of the branches, and not their value, just write this as an
if
> statement.

Done.

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:132:
LogCollectionStatus(DEBUG_FILE_DELETION_FAILED);
On 2017/06/05 15:39:39, Sigurður Ásgeirsson wrote:
> Out of curiosity - assuming a file is somehow permanently stuck (like marked
> read-only, or has an impenetrable ACL) it would accrue counts here for
> perpetuity?
> I'm not saying that's a problem, just wondering.

That is correct.

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.h (right):

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.h:32: class
PostmortemProcessor {
On 2017/06/05 15:39:39, Sigurður Ásgeirsson wrote:
> I don't see that hoisting an interface here is helping you in any way, shape
or
> form. I don't see any case where you're using polymorphic dispatch to these
> classes - it's always instantiate one or the other, then call it.
> 
> I'd say just remove this interface have two unrelated classes handling the two
> cases, unless I've missed something?

I initially thought there'd be a factory, but didn't do it in the end. Removed. 

Done.

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

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/postmortem_report_collector_unittest.cc:93:
DEBUG_FILE_DELETION_FAILED, 1);
On 2017/06/05 15:39:39, Sigurður Ásgeirsson wrote:
> nice. Stupid question, though - wouldn't the Postmortem reporter class satisfy
> this test as-written just fine? Maybe add a negative test to make sure it's
not
> updating the counts it shouldn't?

Done.

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
File components/browser_watcher/stability_paths.h (right):

https://codereview.chromium.org/2923523002/diff/60001/components/browser_watc...
components/browser_watcher/stability_paths.h:42: // excluding those in
|excluded_stability_files.
On 2017/06/05 15:39:39, Sigurður Ásgeirsson wrote:
> nit: closing bar missing.

Done.

[Sigurður Ásgeirsson, 2017-06-05 18:09:00]

lgtm

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

https://codereview.chromium.org/2923523002/diff/80001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:65: if
(base::DeleteFile(file, false)) {
ubernit: I believe our coding guidelines specify no braces unless the
conditional or one side of the branch is multi-line?

[manzagop (departed), 2017-06-05 18:26:26]

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

[manzagop (departed), 2017-06-05 18:26:34]

Thanks for the review!

[commit-bot: I haz the power, 2017-06-05 18:26:48]

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-06-05 18:31:08]

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

[manzagop (departed), 2017-06-05 18:31:09]

Hi Rob!

Could you have an OWNERS' look for tools\metrics\histograms\histograms.xml?

Note I've also introduced an experiment param in:
  watcher_metrics_provider_win.cc

Thanks!

Pierre-Antoine

[commit-bot: I haz the power, 2017-06-05 20:16:27]

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

[commit-bot: I haz the power, 2017-06-05 20:16:28]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-06-05 21:04:03]

lgtm

[manzagop (departed), 2017-06-05 21:05:42]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-06-05 21:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/2923523002/#ps100001
(title: "get rid of some braces")

[manzagop (departed), 2017-06-05 21:05:54]

Thanks!

[commit-bot: I haz the power, 2017-06-05 21:06:32]

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-06-05 21:12:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496696742496330,
"parent_rev": "043077f6b62d68a2b039cdb277c7960fb532b86e", "commit_rev":
"194acde6446e88d5ac045e9768ef53aea4319def"}

[commit-bot: I haz the power, 2017-06-05 21:12:42]

Description was changed from

==========
Decouple stability instrumentation recording and collection

Introduces an experiment parameter to control postmortem collection.
Depending on the parameter's value, stability files are either
collected or simply deleted.

Additional details:
- UncleanShutdownCount and UncleanSystemCount metrics are deprecated
in favor of additional events in ActivityTracker.Collect.Status. 
This is more memory efficient.

BUG=691595
==========

to

==========
Decouple stability instrumentation recording and collection

Introduces an experiment parameter to control postmortem collection.
Depending on the parameter's value, stability files are either
collected or simply deleted.

Additional details:
- UncleanShutdownCount and UncleanSystemCount metrics are deprecated
in favor of additional events in ActivityTracker.Collect.Status. 
This is more memory efficient.

BUG=691595

Review-Url: https://codereview.chromium.org/2923523002
Cr-Commit-Position: refs/heads/master@{#477085}
Committed:
https://chromium.googlesource.com/chromium/src/+/194acde6446e88d5ac045e9768ef...
==========

[commit-bot: I haz the power, 2017-06-05 21:12:44]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/194acde6446e88d5ac045e9768ef...