Revise system profile prefs clearing

Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial stability log has been sent and it is no longer useful. This is to avoid version misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run (e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982
Committed: https://crrev.com/780bb8b8bfb0485e5263c39c17c4aaa300dcabe1
Cr-Commit-Position: refs/heads/master@{#426788}

[manzagop (departed), 2016-10-20 16:31:17]

Description was changed from

==========
Revise system profile prefs clearing

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

to

==========
Revise system profile prefs clearing

BUG=415982
==========

[manzagop (departed), 2016-10-20 16:34:04]

Description was changed from

==========
Revise system profile prefs clearing

BUG=415982
==========

to

==========
Revise system profile prefs clearing

The current approach is to only clear them if the previous run was unclean.

This CL is:
- to ensure clearing on update
- there doesn't seem to be a need to clear those prefs otherwise

BUG=415982
==========

[manzagop (departed), 2016-10-20 16:35:05]

Description was changed from

==========
Revise system profile prefs clearing

The current approach is to only clear them if the previous run was unclean.

This CL is:
- to ensure clearing on update
- there doesn't seem to be a need to clear those prefs otherwise

BUG=415982
==========

to

==========
Revise system profile prefs clearing

The current approach is to only clear them if the previous run was unclean.

This CL is:
- to ensure clearing on update
- there doesn't seem to be a need to clear those prefs otherwise


BUG=415982
==========

[manzagop (departed), 2016-10-20 16:35:45]

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

[manzagop (departed), 2016-10-20 16:35:46]

Hi Alexei,
I think this makes more sense but may be lacking some context. Thanks!
Pierre

[manzagop (departed), 2016-10-20 16:36:25]

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

[commit-bot: I haz the power, 2016-10-20 16:37:01]

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-10-20 16:46:11]

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Alexei Svitkine (slow), 2016-10-20 16:47:26]

lgtm % comments

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
File components/metrics/metrics_log.h (right):

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_log.h:92: // the UI thread. A synthetic trial is one
that is set up dynamically by code
Thanks for removing references to old params.

Actually, the new comment is still not correct because synthetic trial are
generated synchronously on the UI thread. The reason it takes them is because
they're managed by the metrics services.

I suggest just rewording this to not mention the synchronous / UI thread it.
e.g. "Takes a list of synthetic trial IDs as a parameter. A synthetic trial is
one ..."

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
File components/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_service.cc:587: bool
is_initial_stability_log_required =
Nit: const

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_service.cc:621: // cleared.
I think I do agree with this logic, but the comment should be expanded to
provide more context since originally it wasn't obvious to me that this is
desired.

The reason i think this is OK is because by this point if there stability stats
that warranted creating an initial stability log, those stats would already have
been included in that log. So we're not losing those. But the reason we want to
clear it is if we crash before we have a chance to save the new system profile,
so that those new crashes don't get attributed to the old version.

Can you make the comment include the above reasoning? Thanks!

[manzagop (departed), 2016-10-20 18:36:40]

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

[commit-bot: I haz the power, 2016-10-20 18:37:08]

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-10-20 18:38:37]

Thanks!

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
File components/metrics/metrics_log.h (right):

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_log.h:92: // the UI thread. A synthetic trial is one
that is set up dynamically by code
On 2016/10/20 16:47:26, Alexei Svitkine (slow) wrote:
> Thanks for removing references to old params.
> 
> Actually, the new comment is still not correct because synthetic trial are
> generated synchronously on the UI thread. The reason it takes them is because
> they're managed by the metrics services.
> 
> I suggest just rewording this to not mention the synchronous / UI thread it.
> e.g. "Takes a list of synthetic trial IDs as a parameter. A synthetic trial is
> one ..."

Done.

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
File components/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_service.cc:587: bool
is_initial_stability_log_required =
On 2016/10/20 16:47:26, Alexei Svitkine (slow) wrote:
> Nit: const

Done.

https://chromiumcodereview.appspot.com/2428413005/diff/1/components/metrics/m...
components/metrics/metrics_service.cc:621: // cleared.
On 2016/10/20 16:47:26, Alexei Svitkine (slow) wrote:
> I think I do agree with this logic, but the comment should be expanded to
> provide more context since originally it wasn't obvious to me that this is
> desired.
> 
> The reason i think this is OK is because by this point if there stability
stats
> that warranted creating an initial stability log, those stats would already
have
> been included in that log. So we're not losing those. But the reason we want
to
> clear it is if we crash before we have a chance to save the new system
profile,
> so that those new crashes don't get attributed to the old version.
> 
> Can you make the comment include the above reasoning? Thanks!

Done.

[Alexei Svitkine (slow), 2016-10-20 18:42:41]

lgtm

[commit-bot: I haz the power, 2016-10-20 18:51:23]

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

[commit-bot: I haz the power, 2016-10-20 18:51:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[manzagop (departed), 2016-10-20 21:28:01]

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

[commit-bot: I haz the power, 2016-10-20 21:28:28]

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-10-20 21:32:14]

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

[commit-bot: I haz the power, 2016-10-20 21:32:51]

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-10-20 21:37:10]

Description was changed from

==========
Revise system profile prefs clearing

The current approach is to only clear them if the previous run was unclean.

This CL is:
- to ensure clearing on update
- there doesn't seem to be a need to clear those prefs otherwise


BUG=415982
==========

to

==========
Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial
stability log has been sent and it is no longer useful. This is to avoid version
misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run
(e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982
==========

[manzagop (departed), 2016-10-20 21:44:54]

Two tests were failing (my bad!) so I made these changes:

- changed MetricsLogTest.LoadSavedEnvironmentFromPrefs to no longer expect the
clearing of the pref

- updated LoadSavedEnvironmentFromPrefs's comment

- MetricsServiceTest.InitialStabilityLogAfterCrash requires that provider's
HasInitialStabilityMetrics be called even if other conditions already indicate
their presence, so I switched it back to not bypassing
ProvidersHaveInitialStabilityMetrics if the shutdown is unclean. See
https://cs.chromium.org/chromium/src/components/metrics/metrics_service_unitt...

[commit-bot: I haz the power, 2016-10-20 22:49:10]

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

[commit-bot: I haz the power, 2016-10-20 22:49:11]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2016-10-21 14:13:18]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2016-10-21 14:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2428413005/#ps60001
(title: "Update LoadSavedEnvironmentFromPrefs's comment")

[commit-bot: I haz the power, 2016-10-21 14:13:37]

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-10-21 14:21:44]

Description was changed from

==========
Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial
stability log has been sent and it is no longer useful. This is to avoid version
misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run
(e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982
==========

to

==========
Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial
stability log has been sent and it is no longer useful. This is to avoid version
misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run
(e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982
==========

[commit-bot: I haz the power, 2016-10-21 14:21:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 14:30:52]

Description was changed from

==========
Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial
stability log has been sent and it is no longer useful. This is to avoid version
misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run
(e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982
==========

to

==========
Revise system profile prefs clearing

The previous approach was to only clear them if the previous run was unclean.

This CL changes the clearing to every time there is an update, after the initial
stability log has been sent and it is no longer useful. This is to avoid version
misattribution if the updated version crashes before writing a system profile.

Note however that some parts of the system profile may change from run to run
(e.g. field trials). That said, this CL doesn't change behavior in that respect.

BUG=415982

Committed: https://crrev.com/780bb8b8bfb0485e5263c39c17c4aaa300dcabe1
Cr-Commit-Position: refs/heads/master@{#426788}
==========

[commit-bot: I haz the power, 2016-10-21 14:30:53]

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