Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528033003:
MetricsService: ensure in-memory/pref execution phase are in sync (Closed)
|
Created:
4 years ago by manzagop (departed) Modified:
4 years ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMetricsService: ensure in-memory/pref execution phase are in sync
Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen.
- Mobile
In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so.
- LogCleanShutdown
Now also updates MetricsService::execution_phase_ (in addition to
the pref) by calling MetricsService::SetExecutionPhase
- ClearSavedStabilityMetrics now sets the pref to
|execution_phase_| instead of UNINITIALIZED_PHASE.
It equivalent right now, but it seems more correct.
BUG=668542
Committed: https://crrev.com/7c14de297e7610734444f2bbc99141810e8e7b44
Cr-Commit-Position: refs/heads/master@{#434518}
Patch Set 1 #Patch Set 2 : Add metric owner #
Total comments: 4
Patch Set 3 : Address comments #
Total comments: 1
Messages
Total messages: 32 (25 generated)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MetricsService: ensure in-memory/pref execution phase are in sync Details: - ClearSavedStabilityMetrics now sets kStabilityExecutionPhase to MetricsService::execution_phase_ instead of UNINITIALIZED_PHASE (should be a noop with current code) - LogCleanShutdown and MarkAppCleanShutdownAndCommit now also update MetricsService::execution_phase_ (in addition to prefs::kStabilityExecutionPhase) by calling MetricsService::SetExecutionPhase BUG= ========== to ========== MetricsService: ensure in-memory/pref execution phase are in sync Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen. - Mobile In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so. - LogCleanShutdown Now also updates MetricsService::execution_phase_ (in addition to the pref) by calling MetricsService::SetExecutionPhase - ClearSavedStabilityMetrics now sets the pref to |execution_phase_| instead of UNINITIALIZED_PHASE. It equivalent right now, but it seems more correct. BUG=668542 ==========
Patchset #1 (id:1) has been deleted
manzagop@chromium.org changed reviewers: + asvitkine@chromium.org
Hi Alexei! This should mostly make a difference on mobile. Right now the execution phase metric isn't very useful, but it still feels like a good safety rail to have. Perhaps there are ways to improve it in subsequent CLs. Thanks, Pierre
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:484: // Restore the execution phase stored in prefs. Nit: Mention MarkAppCleanShutdownAndCommit() here. https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:527: local_state_->SetInteger(prefs::kStabilityExecutionPhase, execution_phase_); I'm not sure this makes sense. Maybe better to just not set it?
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks! Another look? https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:484: // Restore the execution phase stored in prefs. On 2016/11/24 21:20:20, Alexei Svitkine (slow) wrote: > Nit: Mention MarkAppCleanShutdownAndCommit() here. Done. https://codereview.chromium.org/2528033003/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:527: local_state_->SetInteger(prefs::kStabilityExecutionPhase, execution_phase_); On 2016/11/24 21:20:20, Alexei Svitkine (slow) wrote: > I'm not sure this makes sense. Maybe better to just not set it? Done. The value needs to be preserved up to when it's consumed in MetricsService::InitializeMetricsState. https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?sq... But it's different from the other metrics, in that it's not an accumulation metric. After that, it gets shortly after overwritten to START_METRICS_RECORDING (and this period it outside the uma crash mechanism coverage). https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?sq... So I think it's fine to not set it. https://codereview.chromium.org/2528033003/diff/100001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2528033003/diff/100001/components/metrics/met... components/metrics/metrics_service.cc:593: SetExecutionPhase(UNINITIALIZED_PHASE, local_state_); I moved this here, because is should only be sent for unclean shutdowns. I ended up also resetting the execution phase.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480087470452170,
"parent_rev": "ae4a3d6e3275d85517028cf703cf117b30c39e3a", "commit_rev":
"3b464e482fd75e5be2cd06bba97f96a1a3cdd858"}
Message was sent while issue was closed.
Description was changed from ========== MetricsService: ensure in-memory/pref execution phase are in sync Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen. - Mobile In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so. - LogCleanShutdown Now also updates MetricsService::execution_phase_ (in addition to the pref) by calling MetricsService::SetExecutionPhase - ClearSavedStabilityMetrics now sets the pref to |execution_phase_| instead of UNINITIALIZED_PHASE. It equivalent right now, but it seems more correct. BUG=668542 ========== to ========== MetricsService: ensure in-memory/pref execution phase are in sync Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen. - Mobile In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so. - LogCleanShutdown Now also updates MetricsService::execution_phase_ (in addition to the pref) by calling MetricsService::SetExecutionPhase - ClearSavedStabilityMetrics now sets the pref to |execution_phase_| instead of UNINITIALIZED_PHASE. It equivalent right now, but it seems more correct. BUG=668542 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MetricsService: ensure in-memory/pref execution phase are in sync Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen. - Mobile In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so. - LogCleanShutdown Now also updates MetricsService::execution_phase_ (in addition to the pref) by calling MetricsService::SetExecutionPhase - ClearSavedStabilityMetrics now sets the pref to |execution_phase_| instead of UNINITIALIZED_PHASE. It equivalent right now, but it seems more correct. BUG=668542 ========== to ========== MetricsService: ensure in-memory/pref execution phase are in sync Background: MetricsService contains an |execution_phase_| which is mirrored to prefs::kStabilityExecutionPhase. In some cases, the mirroring doesn't happen. - Mobile In OnAppEnterBackground we call MarkAppCleanShutdownAndCommit which stores SHUTDOWN_COMPLETE to the pref but leaves |execution_phase_| unchanged. That seems ok. However, we restore the pref back to |execution_phase_| in OnAppEnterForeground. This CL does so. - LogCleanShutdown Now also updates MetricsService::execution_phase_ (in addition to the pref) by calling MetricsService::SetExecutionPhase - ClearSavedStabilityMetrics now sets the pref to |execution_phase_| instead of UNINITIALIZED_PHASE. It equivalent right now, but it seems more correct. BUG=668542 Committed: https://crrev.com/7c14de297e7610734444f2bbc99141810e8e7b44 Cr-Commit-Position: refs/heads/master@{#434518} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7c14de297e7610734444f2bbc99141810e8e7b44 Cr-Commit-Position: refs/heads/master@{#434518} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
