Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531123002:
Record MetricsService's execution phase to the stability file (Closed)
|
Created:
4 years ago by manzagop (departed) Modified:
4 years ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the dependency from components/metrics started causing win only targets to be built on other platforms.
BUG=620813
Committed: https://crrev.com/6d14991597586d547ba5f8cd2cd3ee4155606d32
Cr-Commit-Position: refs/heads/master@{#439536}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Make some build targets win only #Patch Set 4 : Merge #Patch Set 5 : Merge #Patch Set 6 : Revise the key #Patch Set 7 : Missing dependency #Patch Set 8 : Merge #Patch Set 9 : Merge #Patch Set 10 : fix issue with incorrect kStabilityExecutionPhase #Messages
Total messages: 87 (54 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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
manzagop@chromium.org changed reviewers: + bcwhite@chromium.org
A first stab at using the new recording.
https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... File components/browser_watcher/stability_debugging_win.cc (right): https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... components/browser_watcher/stability_debugging_win.cc:94: base::debug::ActivityUserData& global_user_data = global_tracker->user_data(); Blank line above. https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... components/browser_watcher/stability_debugging_win.cc:95: global_user_data.SetInt(name, value); Skip assigning it to a variable and just call SetInt(...) directly. https://codereview.chromium.org/2531123002/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2531123002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:165: #include "components/browser_watcher/stability_debugging_win.h" Can you use a generic stability_debugging.h with platform-specific methods having individual #ifdef around them? Then you wouldn't have to put #ifdef in all the files that are going to call 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...
Thanks! Another look? https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... File components/browser_watcher/stability_debugging_win.cc (right): https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... components/browser_watcher/stability_debugging_win.cc:94: base::debug::ActivityUserData& global_user_data = global_tracker->user_data(); On 2016/12/01 16:35:20, bcwhite wrote: > Blank line above. Done. https://codereview.chromium.org/2531123002/diff/1/components/browser_watcher/... components/browser_watcher/stability_debugging_win.cc:95: global_user_data.SetInt(name, value); On 2016/12/01 16:35:20, bcwhite wrote: > Skip assigning it to a variable and just call SetInt(...) directly. Done. https://codereview.chromium.org/2531123002/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2531123002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:165: #include "components/browser_watcher/stability_debugging_win.h" On 2016/12/01 16:35:20, bcwhite wrote: > Can you use a generic stability_debugging.h with platform-specific methods > having individual #ifdef around them? > > Then you wouldn't have to put #ifdef in all the files that are going to call it. Done.
Description was changed from
==========
Record MetricsService's execution phase to the stability file
BUG=620813
==========
to
==========
Record MetricsService's execution phase to the stability file
Also upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
BUG=620813
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
LGTM https://codereview.chromium.org/2531123002/diff/20001/components/browser_watc... File components/browser_watcher/stability_debugging.cc (right): https://codereview.chromium.org/2531123002/diff/20001/components/browser_watc... components/browser_watcher/stability_debugging.cc:24: bool GetCreationTime(const base::Process& process, base::Time* time) { Are these really windows-only?
Any clue why the bots are failing? I'm not understanding why the code it's trying to build (which is windows only) gets pulled in. I'm playing around with minifying it here: https://codereview.chromium.org/2552273002/ https://codereview.chromium.org/2531123002/diff/20001/components/browser_watc... File components/browser_watcher/stability_debugging.cc (right): https://codereview.chromium.org/2531123002/diff/20001/components/browser_watc... components/browser_watcher/stability_debugging.cc:24: bool GetCreationTime(const base::Process& process, base::Time* time) { On 2016/12/05 17:20:57, bcwhite wrote: > Are these really windows-only? The current implementation is, because it relies on ::GetProcessTimes.
On 2016/12/06 20:35:08, manzagop wrote: > Any clue why the bots are failing? I'm not understanding why the code it's > trying to build (which is windows only) gets pulled in. My current hypothesis is that depending on target T in build file B causes *on some bots* all targets from B to be compiled. I've built a minimal example and am asking for validation in https://chromiumcodereview.appspot.com/2550093008/
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
GN learns about targets by starting with a root file (//BUILD.gn) and recursively pulling files in as the targets in the root file refer to them. When GN loads a file (in the default toolchain), *any* target in that file is defined (targets in other toolchains are only defined as needed). So, when you try to build "all", it will build every target that was defined in every file that got pulled in. On the tryservers, we run the "analyze" step to figure out which of the requested targets are actually affected by the files changed in the patch. However, the implementation of analyze isn't currently smart enough to know how to handle changes to GN files themselves, so it bails out and tells the builder to build everything that it thought it might need. So, these builders are configured to build "all", and you changed a build file, so we tried to build everything, and everything included postmortem_minidump_writer because it wasn't explicitly hidden behind an `if` conditional to indicate that it is windows-only. Let me know if that all doesn't make sense.
That makes total sense! TIL. Thanks Dirk!
manzagop@chromium.org changed reviewers: + rkaplow@chromium.org
Hi Rob! Could you have an OWNERS' look for the following files? chrome\browser\chrome_browser_field_trials_desktop.cc chrome\browser\metrics\chrome_metrics_service_client.cc components\metrics\BUILD.gn components\metrics\DEPS components\metrics\metrics_service.cc Thanks! Pierre
Description was changed from
==========
Record MetricsService's execution phase to the stability file
Also upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
BUG=620813
==========
to
==========
Record MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the
dependency from components/metrics started causing win only targets to be built
on other platforms.
BUG=620813
==========
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
Thanks!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org Link to the patchset: https://codereview.chromium.org/2531123002/#ps120001 (title: "Missing dependency")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2531123002/#ps140001 (title: "Merge")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I'm not sure why this is failing.
On 2016/12/16 19:06:47, manzagop wrote:
> I'm not sure why this is failing.
E 59.884s run_tests_on_device(03f9fb5100622c25) Error not bootstrapped.
Failed to start logdog: Missing project [LOGDOG_STREAM_PROJECT]
Traceback (most recent call last):
File
"/b/swarm_slave/w/irgg8hdZ/build/android/pylib/android/logdog_logcat_monitor.py",
line 28, in __init__
self._stream_client = bootstrap.ButlerBootstrap.probe().stream_client()
File
"/b/swarm_slave/w/irgg8hdZ/tools/swarming_client/libs/logdog/bootstrap.py", line
48, in probe
raise NotBootstrappedError('Missing project [%s]' % (cls._ENV_PROJECT,))
Weren't you adjusting build tagets?
On 2016/12/16 20:27:24, bcwhite wrote:
> On 2016/12/16 19:06:47, manzagop wrote:
> > I'm not sure why this is failing.
>
> E 59.884s run_tests_on_device(03f9fb5100622c25) Error not bootstrapped.
> Failed to start logdog: Missing project [LOGDOG_STREAM_PROJECT]
> Traceback (most recent call last):
> File
>
"/b/swarm_slave/w/irgg8hdZ/build/android/pylib/android/logdog_logcat_monitor.py",
> line 28, in __init__
> self._stream_client = bootstrap.ButlerBootstrap.probe().stream_client()
> File
> "/b/swarm_slave/w/irgg8hdZ/tools/swarming_client/libs/logdog/bootstrap.py",
line
> 48, in probe
> raise NotBootstrappedError('Missing project [%s]' % (cls._ENV_PROJECT,))
Yeah the log is litered with those, but I got the impression it's not the cause
reading http://crbug.com/670817
> Weren't you adjusting build tagets?
I simply checked the CQ button...
On 2016/12/16 20:27:24, bcwhite wrote:
> On 2016/12/16 19:06:47, manzagop wrote:
> > I'm not sure why this is failing.
>
> E 59.884s run_tests_on_device(03f9fb5100622c25) Error not bootstrapped.
> Failed to start logdog: Missing project [LOGDOG_STREAM_PROJECT]
> Traceback (most recent call last):
> File
>
"/b/swarm_slave/w/irgg8hdZ/build/android/pylib/android/logdog_logcat_monitor.py",
> line 28, in __init__
> self._stream_client = bootstrap.ButlerBootstrap.probe().stream_client()
> File
> "/b/swarm_slave/w/irgg8hdZ/tools/swarming_client/libs/logdog/bootstrap.py",
line
> 48, in probe
> raise NotBootstrappedError('Missing project [%s]' % (cls._ENV_PROJECT,))
Yeah the log is litered with those, but I got the impression it's not the cause
reading http://crbug.com/670817
> Weren't you adjusting build tagets?
I simply checked the CQ button...
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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: This issue passed the CQ dry run.
Yikes, it seems there was an actual issue. I don't think the logs contained the test output though due to the logdog errors. Found it by code inspection.
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2531123002/#ps180001 (title: "fix issue with incorrect kStabilityExecutionPhase")
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": 180001, "attempt_start_ts": 1482178238257160,
"parent_rev": "67f2a143eabac6e9b8cd37da6233db38f3688c97", "commit_rev":
"1cb5b2d5e28fcf87f50ab7e3d23a9a03928db6d4"}
Message was sent while issue was closed.
Description was changed from
==========
Record MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the
dependency from components/metrics started causing win only targets to be built
on other platforms.
BUG=620813
==========
to
==========
Record MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the
dependency from components/metrics started causing win only targets to be built
on other platforms.
BUG=620813
Review-Url: https://codereview.chromium.org/2531123002
==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from
==========
Record MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the
dependency from components/metrics started causing win only targets to be built
on other platforms.
BUG=620813
Review-Url: https://codereview.chromium.org/2531123002
==========
to
==========
Record MetricsService's execution phase to the stability file
Also:
- upstream some "ifdef(OS_WIN)" by moving stability_debugging_win.{h,cc} to
stability_debugging.{h,cc}.
- enclose components/browser_watcher/BUILD.gn targets in "if (is_win) {}" as the
dependency from components/metrics started causing win only targets to be built
on other platforms.
BUG=620813
Committed: https://crrev.com/6d14991597586d547ba5f8cd2cd3ee4155606d32
Cr-Commit-Position: refs/heads/master@{#439536}
==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6d14991597586d547ba5f8cd2cd3ee4155606d32 Cr-Commit-Position: refs/heads/master@{#439536} |
