Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1748403003:
Log histograms on shutdown when verbose logging is on (Closed)
|
Created:
4 years, 9 months ago by Boris Yusupov Modified:
4 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog histograms on shutdown when verbose logging is on
It is working in tests but isn't in browser.
In browser initialization of metrics recorder is made from content
earlier than logging which is made from chrome.
Here we turn histograms recording on manually from chrome without
changing none of startup events.
R=asvitkine@chromium.org
BUG=
Committed: https://crrev.com/648b3732cd93b242fd34114dca2915b0ff31f675
Cr-Commit-Position: refs/heads/master@{#403752}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review issues #Patch Set 3 : Remove abandoned test class #
Total comments: 1
Patch Set 4 : added comment and friend #
Total comments: 2
Patch Set 5 : typo #
Messages
Total messages: 31 (12 generated)
On 2016/03/01 14:58:06, Boris Yusupov wrote: Please review
Sorry for the delay in response. I haven't had much time to look at this - but can do so next week. However, I don't really understand the motivation for this. Can you elaborate on why you're doing it?
On 2016/03/24 17:39:49, Alexei Svitkine (OOO 25-27th) wrote: > Sorry for the delay in response. I haven't had much time to look at this - but > can do so next week. > > However, I don't really understand the motivation for this. Can you elaborate on > why you're doing it? This is useful for automatization testing, sometimes our automatization and performance engineers want it when they looking something after shutdown process. It's possible in unit or browser tests but doesn't work in real browser and thus in auto tests. We can overcome it, but with log records it's just easier. If it will bring troubles we can easily don't do this but now it looks just like little inconsistency.
On 2016/03/24 17:39:49, Alexei Svitkine wrote: > Sorry for the delay in response. I haven't had much time to look at this - but > can do so next week. > > However, I don't really understand the motivation for this. Can you elaborate on > why you're doing it? Well, what do you think?
Sorry it has taken me so long to get to this. Finally got a chance to review it. https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... base/metrics/statistics_recorder.cc:25: bool g_vlog_initialized = false; Can this just be a member of the StatisticsRecorder? https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... File base/metrics/statistics_recorder_unittest.cc (right): https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... base/metrics/statistics_recorder_unittest.cc:67: class LogStateSaver { Put this in an anon namespace at the top of the file. https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... base/metrics/statistics_recorder_unittest.cc:90: LogStateSaver log_state_saver_; Is there any problem doing this unconditionally for every test? If so, you can then remove all these other test classes. https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... base/metrics/statistics_recorder_unittest.cc:93: class StatisticsRecorderLoggingWarningOnStartupTest You don't need to create a separate test harness class for each test. Just use StatisticsRecorderTest.
On 2016/04/14 20:47:39, Alexei Svitkine wrote: > Sorry it has taken me so long to get to this. Finally got a chance to review it. > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > File base/metrics/statistics_recorder.cc (right): > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > base/metrics/statistics_recorder.cc:25: bool g_vlog_initialized = false; > Can this just be a member of the StatisticsRecorder? > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > File base/metrics/statistics_recorder_unittest.cc (right): > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > base/metrics/statistics_recorder_unittest.cc:67: class LogStateSaver { > Put this in an anon namespace at the top of the file. > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > base/metrics/statistics_recorder_unittest.cc:90: LogStateSaver log_state_saver_; > Is there any problem doing this unconditionally for every test? If so, you can > then remove all these other test classes. > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > base/metrics/statistics_recorder_unittest.cc:93: class > StatisticsRecorderLoggingWarningOnStartupTest > You don't need to create a separate test harness class for each test. Just use > StatisticsRecorderTest. Changed everything as you suggested
On 2016/05/13 13:29:03, Boris Yusupov wrote: > On 2016/04/14 20:47:39, Alexei Svitkine wrote: > > Sorry it has taken me so long to get to this. Finally got a chance to review > it. > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > File base/metrics/statistics_recorder.cc (right): > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > base/metrics/statistics_recorder.cc:25: bool g_vlog_initialized = false; > > Can this just be a member of the StatisticsRecorder? > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > File base/metrics/statistics_recorder_unittest.cc (right): > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > base/metrics/statistics_recorder_unittest.cc:67: class LogStateSaver { > > Put this in an anon namespace at the top of the file. > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > base/metrics/statistics_recorder_unittest.cc:90: LogStateSaver > log_state_saver_; > > Is there any problem doing this unconditionally for every test? If so, you can > > then remove all these other test classes. > > > > > https://codereview.chromium.org/1748403003/diff/1/base/metrics/statistics_rec... > > base/metrics/statistics_recorder_unittest.cc:93: class > > StatisticsRecorderLoggingWarningOnStartupTest > > You don't need to create a separate test harness class for each test. Just use > > StatisticsRecorderTest. > > Changed everything as you suggested Ping :)
lgtm, sorry for the delay https://codereview.chromium.org/1748403003/diff/40001/base/metrics/statistics... File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1748403003/diff/40001/base/metrics/statistics... base/metrics/statistics_recorder.h:222: void InitLogOnShutdownWithoutLock(); Nit: Add a comment.
The CQ bit was checked by boriay@yandex-team.ru
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/1748403003/#ps60001 (title: "added comment and friend")
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
The author boriay@yandex-team.ru has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by boriay@yandex-team.ru
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of sturtup events. R=asvitkine@chromium.org BUG= ========== to ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of sturtup events. R=asvitkine@chromium.org BUG= ==========
boriay@yandex-team.ru changed reviewers: + thestig@chromium.org
Oh, missed reviewer for logging_chrome.cc thus added Lei. Please review.
Description was changed from ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of sturtup events. R=asvitkine@chromium.org BUG= ========== to ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of startup events. R=asvitkine@chromium.org BUG= ==========
lgtm, but please address the nits below: https://codereview.chromium.org/1748403003/diff/60001/base/metrics/statistics... File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1748403003/diff/60001/base/metrics/statistics... base/metrics/statistics_recorder.h:170: // Initializes logging histograms with --v=1. Safe to call several times. s/several/multiple/ https://codereview.chromium.org/1748403003/diff/60001/base/metrics/statistics... base/metrics/statistics_recorder.h:216: // StatisticsRecorder by itlsef if needed (it isn't in unit tests). type: "itself"
The CQ bit was checked by boriay@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1748403003/#ps80001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of startup events. R=asvitkine@chromium.org BUG= ========== to ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of startup events. R=asvitkine@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of startup events. R=asvitkine@chromium.org BUG= ========== to ========== Log histograms on shutdown when verbose logging is on It is working in tests but isn't in browser. In browser initialization of metrics recorder is made from content earlier than logging which is made from chrome. Here we turn histograms recording on manually from chrome without changing none of startup events. R=asvitkine@chromium.org BUG= Committed: https://crrev.com/648b3732cd93b242fd34114dca2915b0ff31f675 Cr-Commit-Position: refs/heads/master@{#403752} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/648b3732cd93b242fd34114dca2915b0ff31f675 Cr-Commit-Position: refs/heads/master@{#403752} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
