Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6486021:
Collect some disk statistics. (Closed)
|
Created:
9 years, 10 months ago by Luigi Semenzato Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, sosa, petkov, Luigi Semenzato Visibility:
Public. |
DescriptionCollect some disk statistics.
Change-Id: Id30f4b7e5d121f2632592ebacf47a18ea1d89fec
BUG=chromium-os:12171
TEST=ran on target and observed that stats are generated
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=78d682c
Patch Set 1 #Patch Set 2 : remove duplicate log and close file #
Total comments: 14
Patch Set 3 : Fixed style and added tests #
Total comments: 16
Patch Set 4 : avoid #define, goto, blank lines #
Total comments: 9
Patch Set 5 : cleanup from review #
Total comments: 7
Patch Set 6 : More small fixes. #Patch Set 7 : Change metrix prefix Filesystem back to Platform #
Messages
Total messages: 18 (0 generated)
Darin, you are the main reviewer. Mandeep and Stephan: please give feedback on the kind of collected data and the frequency (feel free to give feedback on the code too). This is just a starting point. This code collects samples at 30s intervals. There are four samples, all of them in units of sectors per second: sectors read/written in the last 30 seconds, and sectors read/written in the last second. (Note that we collect the latter every 30s, not every 1s.) The data is immediately available under about:histograms and is sent to the UMA servers. This should give us a rough baseline for disk activity in the field.
Some comments below. Care to add some unit tests to metrics_daemon_test.cc? You can also run the unit tests through FEATURES=test emerge... or through cros_run_unit_tests. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode90 metrics_daemon.cc:90: "Platform.ReadSectorsLong"; maybe add a comment about the units -- sectors/second? is a sector 512 bytes? http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode98 metrics_daemon.cc:98: const int MetricsDaemon::kMetricDiskStatsShortInterval = 1; // seconds just two spaces before // http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode106 metrics_daemon.cc:106: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; x86 specific... is it easy to make this work for ARM too? http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode529 metrics_daemon.cc:529: diskstats_source_ = g_timeout_source_new_seconds(wait); You don't really need to store the source for this sort of timeout scheduling because you're never destroying the source directly. So, the 3 lines can be collapsed into a single line: g_timeout_add_seconds(wait, DiskStatsCallbackStatic, this); http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode539 metrics_daemon.cc:539: int file = open(kMetricsDiskStatsPath, O_RDONLY); want to use HANDLE_EINTR macro here and below for the syscalls? see counter.cc. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode540 metrics_daemon.cc:540: LOG_IF(FATAL, file < 0) << "cannot open " << kMetricsDiskStatsPath << ": " FATAL? This would cause the process to crash every 30 seconds in case this fails... Actually, right now it means the process will crash every 30 seconds on ARM. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode556 metrics_daemon.cc:556: return false; // destroy this source instance just 2 spaces before // http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode565 metrics_daemon.cc:565: case kDiskStatsShort: per style, case needs to be 2-space indented relative to switch; case body 2-space indented relative to case. don't you use emacs with google style configuration? :-) http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.h File metrics_daemon.h (right): http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.h#newcode306 metrics_daemon.h:306: remove blank line
On 2011/02/11 06:42:30, petkov wrote: > Some comments below. Care to add some unit tests to metrics_daemon_test.cc? You > can also run the unit tests through FEATURES=test emerge... or through > cros_run_unit_tests. Thank you, I think I have addressed all issues. PTAL!
http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode106 metrics_daemon.cc:106: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; On 2011/02/11 06:42:30, petkov wrote: > x86 specific... is it easy to make this work for ARM too? ARM uses the same path, at least on my machine. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode529 metrics_daemon.cc:529: diskstats_source_ = g_timeout_source_new_seconds(wait); On 2011/02/11 06:42:30, petkov wrote: > You don't really need to store the source for this sort of timeout scheduling > because you're never destroying the source directly. So, the 3 lines can be > collapsed into a single line: > > g_timeout_add_seconds(wait, DiskStatsCallbackStatic, this); Splendid, thanks! http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode539 metrics_daemon.cc:539: int file = open(kMetricsDiskStatsPath, O_RDONLY); On 2011/02/11 06:42:30, petkov wrote: > want to use HANDLE_EINTR macro here and below for the syscalls? see counter.cc. Thanks, good suggestion. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode540 metrics_daemon.cc:540: LOG_IF(FATAL, file < 0) << "cannot open " << kMetricsDiskStatsPath << ": " On 2011/02/11 06:42:30, petkov wrote: > FATAL? This would cause the process to crash every 30 seconds in case this > fails... Actually, right now it means the process will crash every 30 seconds on > ARM. You are completely right. Making non-fatal. http://codereview.chromium.org/6486021/diff/3001/metrics_daemon.cc#newcode565 metrics_daemon.cc:565: case kDiskStatsShort: On 2011/02/11 06:42:30, petkov wrote: > per style, case needs to be 2-space indented relative to switch; case body > 2-space indented relative to case. don't you use emacs with google style > configuration? :-) I was overriding some indentation variables by mistake. It works now, thanks.
A few more comments.. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode111 metrics_daemon.cc:111: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; Unused? It seems that the current revision won't work on the device -- the path field is uninitialized. Also, I'm really surprised that ARM exposes the stat through "sda" -- I know for sure that root device is not /dev/sda... But I can't really double check so I trust you :-) http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode558 metrics_daemon.cc:558: goto close_file; goto is a bit evil. I'd suggestion two options: - define a helper routine that you invoke after opening the file, passing in the file descriptor, and then close the descriptor in the caller. - define and use a FileCloser class that gets the file descriptor as an input and closes the file in its destructor. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode582 metrics_daemon.cc:582: remove blank line? http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode587 metrics_daemon.cc:587: 0, I think 0 is invalid minimum for some reason-- minimum is 1. Maybe it just works... Please double check histogram.h. Same below. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.h File metrics_daemon.h (right): http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.h#newcode33 metrics_daemon.h:33: void Init(bool testing, MetricsLibraryInterface* metrics_lib, Coding style forbids overloading... :/ Just use the 3-argument version and pass the constant path in. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:37: #define READ_SECTORS_0 80000 no #define per style :/ you may need to use StringPrintf or something... http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:146: virtual void TearDown() {} maybe delete the fake stats file? http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:580: EXPECT_CALL(metrics_lib_, SendToUMA(_, _, _, _, _)) You could check for the actual values that get sent, right?
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode111 metrics_daemon.cc:111: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; Isn't this something like /sys/block/mmcblk0 on ARM?
Thanks! I have changed a few items as recommended, but I have questions on a
couple of items.
Also, an interesting thought. The following (untested) script performs the same
function as this CL. It's 1/3 of the size of this CL, but complexity-wise it's
a lot less than that.
#! /bin/sh
function read_stats() {
stats=$(cat /sys/block/sda/stat)
read=$(echo $stats | awk '{print $3}')
write=$(echo $stats | awk '{print $7}')
}
function send_stats() {
metrics_client blah.blah $(($read - $last_read)) 1 1000 50
metrics_client blah.blah $(($write - $last_write)) 1 1000 50
}
read_stats
last_read=$read
last_write=$write
while (1); do
sleep 1
read_stats
send_stats
sleep 29
read_stats
send_stats
last_read=$read
last_write=$write
done
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc
File metrics_daemon.cc (right):
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode111
metrics_daemon.cc:111: const char MetricsDaemon::kMetricsDiskStatsPath[] =
"/sys/class/block/sda/stat";
On 2011/02/13 17:28:47, petkov wrote:
> Isn't this something like /sys/block/mmcblk0 on ARM?
That path exists on my kaen, but the stats there show very little i/o activity,
and don't change if I read from /dev/sda.
I'll have to find a reliable way of telling if we're looking at the right
device, but for now it appears we are.
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon.cc#newcode558
metrics_daemon.cc:558: goto close_file;
On 2011/02/12 05:34:45, petkov wrote:
> goto is a bit evil. I'd suggestion two options:
>
> - define a helper routine that you invoke after opening the file, passing in
the
> file descriptor, and then close the descriptor in the caller.
> - define and use a FileCloser class that gets the file descriptor as an input
> and closes the file in its destructor.
And a third option: an else clause. (It doesn't scale, but it doesn't have to.)
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc
File metrics_daemon_test.cc (right):
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco...
metrics_daemon_test.cc:37: #define READ_SECTORS_0 80000
On 2011/02/12 05:34:45, petkov wrote:
> no #define per style :/ you may need to use StringPrintf or something...
Yes. Too bad that #defines are the only way to statically convert a number to a
string in C.
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco...
metrics_daemon_test.cc:146: virtual void TearDown() {}
On 2011/02/12 05:34:45, petkov wrote:
> maybe delete the fake stats file?
Except that it's useful for debugging. Is it generally recommended/acceptable
for tests to leave state around?
http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco...
metrics_daemon_test.cc:580: EXPECT_CALL(metrics_lib_, SendToUMA(_, _, _, _, _))
On 2011/02/12 05:34:45, petkov wrote:
> You could check for the actual values that get sent, right?
Can I? I don't know how that EXPECT_CALL framework works. In this case there
are two calls to SendToUMA. Can I have two EXPECT_CALLs, each with the expected
argument values?
On 2011/02/14 18:43:43, Luigi Semenzato wrote: > Thanks! I have changed a few items as recommended, but I have questions on a > couple of items. > > Also, an interesting thought. The following (untested) script performs the same > function as this CL. It's 1/3 of the size of this CL, but complexity-wise it's > a lot less than that. Actually more like 1/8. 26 lines versus 222 new lines.
shells scripts are evil for a production system, I think. if you end up doing it in a shell script, you should also develop unit and/or autotests for too. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:146: virtual void TearDown() {} On 2011/02/14 18:43:43, Luigi Semenzato wrote: > On 2011/02/12 05:34:45, petkov wrote: > > maybe delete the fake stats file? > > Except that it's useful for debugging. Is it generally recommended/acceptable > for tests to leave state around? Don't know but from what I've seen, unit tests try to clean up after themselves. http://codereview.chromium.org/6486021/diff/5001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:580: EXPECT_CALL(metrics_lib_, SendToUMA(_, _, _, _, _)) On 2011/02/14 18:43:43, Luigi Semenzato wrote: > On 2011/02/12 05:34:45, petkov wrote: > > You could check for the actual values that get sent, right? > > Can I? I don't know how that EXPECT_CALL framework works. In this case there Yes, you can. Also, right now you're not testing any of the parsing code, really. > are two calls to SendToUMA. Can I have two EXPECT_CALLs, each with the expected > argument values? http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc#newcode94 metrics_daemon.cc:94: "Filesystem.ReadSectorsLong"; I guess you don't like Platform any more? Are you envisioning more metrics under Filesystem? Again, the more groups the longer the dashboard drop down menu... http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc#newcode111 metrics_daemon.cc:111: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; still unused http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.h File metrics_daemon.h (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.h#newcode33 metrics_daemon.h:33: void Init(bool testing, MetricsLibraryInterface* metrics_lib, Again -- no overloading. http://codereview.chromium.org/6486021/diff/12001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:37: #define READ_SECTORS_0 80000 no #define -- maybe change to static consts... http://codereview.chromium.org/6486021/diff/12001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:74: snprintf(kFakeDiskStats[0], sizeof(kFakeDiskStats[0]), Use StringPrintf.
Thanks. > shells scripts are evil for a production system, I think. They can be and they often are. But they have their place. > if you end up doing it in a shell script, > you should also develop unit and/or autotests for too. Sure, and then the code difference wouldn't be as large. But still, I wouldn't test "awk", for instance, whereas here I have to test my own parsing code. In this case there are other advantages to integrating this functionality into metrics_daemon. However, I think that often the best solution is not definable by rules. The problem is that different people have a different idea of what it is... http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc#newcode94 metrics_daemon.cc:94: "Filesystem.ReadSectorsLong"; On 2011/02/14 19:28:33, petkov wrote: > I guess you don't like Platform any more? Are you envisioning more metrics under > Filesystem? Again, the more groups the longer the dashboard drop down menu... > Yeah, I know. Our current plan is to add Filesystem to the drop down menu, but maybe it shouldn't be up to us to decide. http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.cc#newcode111 metrics_daemon.cc:111: const char MetricsDaemon::kMetricsDiskStatsPath[] = "/sys/class/block/sda/stat"; On 2011/02/14 19:28:33, petkov wrote: > still unused Sorry about this. Not enough caffeine. http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.h File metrics_daemon.h (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon.h#newcode33 metrics_daemon.h:33: void Init(bool testing, MetricsLibraryInterface* metrics_lib, On 2011/02/14 19:28:33, petkov wrote: > Again -- no overloading. Sorry. Caffeine underdose. http://codereview.chromium.org/6486021/diff/12001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/12001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:37: #define READ_SECTORS_0 80000 On 2011/02/14 19:28:33, petkov wrote: > no #define -- maybe change to static consts... Sorry. Too little caffeine.
A few more final nits. I guess "Filesystem" is fine although the actual metric has little to do with the Filesystem, per-se. http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode144 metrics_daemon.cc:144: MetricsDaemon::MetricsDaemon() initialize diskstats_path_ to NULL here. http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode578 metrics_daemon.cc:578: 0, did you double check that it's OK for this to be 0 rather than 1? http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc File metrics_daemon_main.cc (right): http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc#newc... metrics_daemon_main.cc:12: // Path to disk stats. This may be system dependend. typo: dependent http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:572: SendToUMA(_, (kFakeReadSectors[1] - kFakeReadSectors[0]) / 30, any reason for not matching the rest of the arguments?
On 2011/02/14 23:39:16, petkov wrote: > A few more final nits. > > I guess "Filesystem" is fine although the actual metric has little to do with > the Filesystem, per-se. I am really not sure. Would you prefer "Platform"? Also "Logging" doesn't have much to do with "KernelDailyCrashes" no? It's just the area where we file (will file) the bug. > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc > File metrics_daemon.cc (right): > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode144 > metrics_daemon.cc:144: MetricsDaemon::MetricsDaemon() > initialize diskstats_path_ to NULL here. > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode578 > metrics_daemon.cc:578: 0, > did you double check that it's OK for this to be 0 rather than 1? > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc > File metrics_daemon_main.cc (right): > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc#newc... > metrics_daemon_main.cc:12: // Path to disk stats. This may be system dependend. > typo: dependent > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc > File metrics_daemon_test.cc (right): > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc#newc... > metrics_daemon_test.cc:572: SendToUMA(_, (kFakeReadSectors[1] - > kFakeReadSectors[0]) / 30, > any reason for not matching the rest of the arguments?
On 2011/02/15 00:09:39, Luigi Semenzato wrote: > On 2011/02/14 23:39:16, petkov wrote: > > A few more final nits. > > > > I guess "Filesystem" is fine although the actual metric has little to do with > > the Filesystem, per-se. > > I am really not sure. Would you prefer "Platform"? Also "Logging" doesn't have > much to do with "KernelDailyCrashes" no? It's just the area where we file (will > file) the bug. It's really up to you -- I'm just pointing out differences, etc. It will be up to you and whoever looks at these metrics to remember to look them up under the right category. > > > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc > > File metrics_daemon.cc (right): > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode144 > > metrics_daemon.cc:144: MetricsDaemon::MetricsDaemon() > > initialize diskstats_path_ to NULL here. > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode578 > > metrics_daemon.cc:578: 0, > > did you double check that it's OK for this to be 0 rather than 1? > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc > > File metrics_daemon_main.cc (right): > > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc#newc... > > metrics_daemon_main.cc:12: // Path to disk stats. This may be system > dependend. > > typo: dependent > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc > > File metrics_daemon_test.cc (right): > > > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc#newc... > > metrics_daemon_test.cc:572: SendToUMA(_, (kFakeReadSectors[1] - > > kFakeReadSectors[0]) / 30, > > any reason for not matching the rest of the arguments?
If it's up to me, LGTY then? On Mon, Feb 14, 2011 at 4:46 PM, <petkov@chromium.org> wrote: > On 2011/02/15 00:09:39, Luigi Semenzato wrote: >> >> On 2011/02/14 23:39:16, petkov wrote: >> > A few more final nits. >> > >> > I guess "Filesystem" is fine although the actual metric has little to do > > with >> >> > the Filesystem, per-se. > >> I am really not sure. Would you prefer "Platform"? Also "Logging" >> doesn't > > have >> >> much to do with "KernelDailyCrashes" no? It's just the area where we file > > (will >> >> file) the bug. > > It's really up to you -- I'm just pointing out differences, etc. It will be > up > to you and whoever looks at these metrics to remember to look them up under > the > right category. > > > >> > >> > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc >> > File metrics_daemon.cc (right): >> > >> > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode144 >> >> > metrics_daemon.cc:144: MetricsDaemon::MetricsDaemon() >> > initialize diskstats_path_ to NULL here. >> > >> > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode578 >> >> > metrics_daemon.cc:578: 0, >> > did you double check that it's OK for this to be 0 rather than 1? >> > >> > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc >> > File metrics_daemon_main.cc (right): >> > >> > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_main.cc#newc... >> >> > metrics_daemon_main.cc:12: // Path to disk stats. This may be system >> dependend. >> > typo: dependent >> > >> > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc >> > File metrics_daemon_test.cc (right): >> > >> > > > http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc#newc... >> >> > metrics_daemon_test.cc:572: SendToUMA(_, (kFakeReadSectors[1] - >> > kFakeReadSectors[0]) / 30, >> > any reason for not matching the rest of the arguments? > > > > http://codereview.chromium.org/6486021/ >
LGTM (a PTAL would have helped to see something changed... :-) )
Oh yeah, my bad! Thanks. On Mon, Feb 14, 2011 at 10:03 PM, <petkov@chromium.org> wrote: > LGTM > > (a PTAL would have helped to see something changed... :-) ) > > > http://codereview.chromium.org/6486021/ >
Forgot to save/publish. Here we go. http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode144 metrics_daemon.cc:144: MetricsDaemon::MetricsDaemon() On 2011/02/14 23:39:16, petkov wrote: > initialize diskstats_path_ to NULL here. Thanks. http://codereview.chromium.org/6486021/diff/16001/metrics_daemon.cc#newcode578 metrics_daemon.cc:578: 0, On 2011/02/14 23:39:16, petkov wrote: > did you double check that it's OK for this to be 0 rather than 1? No, it should be 1. Fixing. http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6486021/diff/16001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:572: SendToUMA(_, (kFakeReadSectors[1] - kFakeReadSectors[0]) / 30, On 2011/02/14 23:39:16, petkov wrote: > any reason for not matching the rest of the arguments? Yes. The other arguments are passed as literal constants. It doesn't seem that I'd be testing anything of substance. (I would verify that 4 == 4 and 4 != 5, for instance). OTOH it may make the test fail unnecessarily upon changes. Unless you feel it's safer to type each numeric constant twice.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
