Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6804014:
Add meminfo UMA collection. (Closed)
|
Created:
9 years, 8 months ago by Luigi Semenzato Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, petkov, sosa Visibility:
Public. |
DescriptionAdd meminfo UMA collection.
Change-Id: Ief779a5bdc68b8e5bf2f1ed979bf30b50aca8e0f
BUG=chromium-os:13747
TEST=verify that Platform.Meminfo* entries are in about:histograms.
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ba3434f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add new fields and logarithmic histograms #
Total comments: 31
Patch Set 3 : various changes from review #Patch Set 4 : . #
Total comments: 11
Patch Set 5 : more fixes #
Messages
Total messages: 11 (0 generated)
Darin please review code. Mandeep and Stephan please comment on numbers collected and frequency of collection (30s) (feel free to review code too, but not required). Thanks!
Maybe add a few more counts. Otherwise, LGTM. http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode681 metrics_daemon.cc:681: // { "Unevictable", "Unevictable" }, Want this. Not percentage. http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode689 metrics_daemon.cc:689: // { "Shmem", "Shmem" }, Want this. Not percentage (luigi's comment) http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode690 metrics_daemon.cc:690: // { "Slab", "Slab" }, Want this. Not percentage (luigi's comment)
Added and tested, PTAL. On 2011/04/06 22:35:26, Mandeep Singh Baines wrote: > Maybe add a few more counts. Otherwise, LGTM. > > http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc > File metrics_daemon.cc (right): > > http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode681 > metrics_daemon.cc:681: // { "Unevictable", "Unevictable" }, > Want this. Not percentage. > > http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode689 > metrics_daemon.cc:689: // { "Shmem", "Shmem" }, > Want this. Not percentage (luigi's comment) > > http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode690 > metrics_daemon.cc:690: // { "Slab", "Slab" }, > Want this. Not percentage (luigi's comment)
LGTM semenzato@chromium.org (semenzato@chromium.org) wrote: > Added and tested, PTAL. > > On 2011/04/06 22:35:26, Mandeep Singh Baines wrote: > >Maybe add a few more counts. Otherwise, LGTM. > > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc > >File metrics_daemon.cc (right): > > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode681 > >metrics_daemon.cc:681: // { "Unevictable", "Unevictable" }, > >Want this. Not percentage. > > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode689 > >metrics_daemon.cc:689: // { "Shmem", "Shmem" }, > >Want this. Not percentage (luigi's comment) > > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode690 > >metrics_daemon.cc:690: // { "Slab", "Slab" }, > >Want this. Not percentage (luigi's comment) > > > > http://codereview.chromium.org/6804014/
Hi Ken. Ping? Thanks!
On 2011/04/06 23:34:54, Mandeep Singh Baines wrote:
> LGTM
>
> mailto:semenzato@chromium.org (mailto:semenzato@chromium.org) wrote:
> > Added and tested, PTAL.
> >
> > On 2011/04/06 22:35:26, Mandeep Singh Baines wrote:
> > >Maybe add a few more counts. Otherwise, LGTM.
> >
> > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc
> > >File metrics_daemon.cc (right):
> >
> > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode681
> > >metrics_daemon.cc:681: // { "Unevictable", "Unevictable" },
> > >Want this. Not percentage.
> >
> > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode689
> > >metrics_daemon.cc:689: // { "Shmem", "Shmem" },
> > >Want this. Not percentage (luigi's comment)
> >
> > >http://codereview.chromium.org/6804014/diff/1/metrics_daemon.cc#newcode690
> > >metrics_daemon.cc:690: // { "Slab", "Slab" },
> > >Want this. Not percentage (luigi's comment)
> >
> >
> >
> > http://codereview.chromium.org/6804014/
http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode641 metrics_daemon.cc:641: int file = HANDLE_EINTR(open(meminfo_path, O_RDONLY)); Consider simplifying this by just using file_util::ReadFileToString - though it doesn't give you the ability to truncate the file at 4K. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode662 metrics_daemon.cc:662: /* This array has one string for every item of /proc/meminfo that we want to Local style is to use // here and below. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode671 metrics_daemon.cc:671: { "MemTotal", "MemTotal" }, // SPECIAL CASE: total system memory This change increases our current number of histograms by a large %. Note that you'll need to add all of these to internal chrome source's xml separately. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode675 metrics_daemon.cc:675: // { "SwapCached", "SwapCached" }, Seems like if you're going to add spaces for alignment (which is not commonly done) you should make sure the commented out lines also line up. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode691 metrics_daemon.cc:691: { "Slab", "Slab", 1 }, Suggest either getting rid of spaces for alignment or making the log_scale columns align too. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode695 metrics_daemon.cc:695: const int nfields = sizeof(fields) / sizeof(fields[0]); arraysize(fields) http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode711 metrics_daemon.cc:711: LOG(WARNING) << "cannot find field " << fields[i].match << with LOG statements, the style is to have wrapped lines start with << which align with the << of the first line. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode720 metrics_daemon.cc:720: p += strlen(fields[i].match); I would suggest finding a way to replace all this pointer/parsing stuff with SplitString('\n') and SplitString(' ') or Tokenize from string_util.h. Less error prone and easier to maintain. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode728 metrics_daemon.cc:728: meminfo_value = (int) strtol(p, &rest, 10); static_cast http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100); This change increases our current number of histograms by a large %. Note that you'll need to add all of these to internal chrome source's xml separately. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100); So, doesn't this create a histogram with 4 million separately countable buckets. Wouldn't this change essentially up the memory usage by 4M*4B*13=208MB ironically to monitor memory usage? :) http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode746 metrics_daemon.cc:746: int percent = meminfo_value * 100 / total_memory; this will crash if the kernel ever doesn't report meminfo as the first line. unlikely, but probably worth a safeguard. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode786 metrics_daemon.cc:786: int max, int nbuckets) { I suggest not passing nbuckets since it's not used and might mislead someone to think it's only using memory proportional to nbuckets. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode792 metrics_daemon.cc:792: metrics_lib_->SendEnumToUMA(name, sample, max); Ignoring the memory usage issue, I thought somewhere you had to document what each value of the "enum" meant for server display. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h File metrics_daemon.h (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h#newcode65 metrics_daemon.h:65: FRIEND_TEST(MetricsDaemonTest, ProcessMeminfo); abc order http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:613: Committed_AS: 1260528 kB\n\ Might be nice to check at least one of these numbers is being parsed correctly, an ignored field is not being reported, and a non-ignored field is.
Thanks! Where are documentation and header files for the string_util.h you mention? On Fri, Apr 8, 2011 at 4:04 PM, <kmixter@chromium.org> wrote: > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc > > File metrics_daemon.cc (right): > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode641 > metrics_daemon.cc:641: int file = HANDLE_EINTR(open(meminfo_path, > O_RDONLY)); > Consider simplifying this by just using file_util::ReadFileToString - > though it doesn't give you the ability to truncate the file at 4K. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode662 > metrics_daemon.cc:662: /* This array has one string for every item of > /proc/meminfo that we want to > Local style is to use // here and below. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode671 > metrics_daemon.cc:671: { "MemTotal", "MemTotal" }, // SPECIAL CASE: > total system memory > This change increases our current number of histograms by a large %. > Note that you'll need to add all of these to internal chrome source's > xml separately. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode675 > metrics_daemon.cc:675: // { "SwapCached", "SwapCached" }, > Seems like if you're going to add spaces for alignment (which is not > commonly done) you should make sure the commented out lines also line > up. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode691 > metrics_daemon.cc:691: { "Slab", "Slab", 1 }, > Suggest either getting rid of spaces for alignment or making the > log_scale columns align too. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode695 > metrics_daemon.cc:695: const int nfields = sizeof(fields) / > sizeof(fields[0]); > arraysize(fields) > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode711 > metrics_daemon.cc:711: LOG(WARNING) << "cannot find field " << > fields[i].match << > with LOG statements, the style is to have wrapped lines start with << > which align with the << of the first line. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode720 > metrics_daemon.cc:720: p += strlen(fields[i].match); > I would suggest finding a way to replace all this pointer/parsing stuff > with SplitString('\n') and SplitString(' ') or Tokenize from > string_util.h. Less error prone and easier to maintain. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode728 > metrics_daemon.cc:728: meminfo_value = (int) strtol(p, &rest, 10); > static_cast > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 > metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * > 1000 * 1000, 100); > This change increases our current number of histograms by a large %. > Note that you'll need to add all of these to internal chrome source's > xml separately. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 > metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * > 1000 * 1000, 100); > So, doesn't this create a histogram with 4 million separately countable > buckets. Wouldn't this change essentially up the memory usage by > 4M*4B*13=208MB ironically to monitor memory usage? :) > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode746 > metrics_daemon.cc:746: int percent = meminfo_value * 100 / total_memory; > this will crash if the kernel ever doesn't report meminfo as the first > line. unlikely, but probably worth a safeguard. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode786 > metrics_daemon.cc:786: int max, int nbuckets) { > I suggest not passing nbuckets since it's not used and might mislead > someone to think it's only using memory proportional to nbuckets. > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode792 > metrics_daemon.cc:792: metrics_lib_->SendEnumToUMA(name, sample, max); > Ignoring the memory usage issue, I thought somewhere you had to document > what each value of the "enum" meant for server display. > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h > File metrics_daemon.h (right): > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h#newcode65 > metrics_daemon.h:65: FRIEND_TEST(MetricsDaemonTest, ProcessMeminfo); > abc order > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc > File metrics_daemon_test.cc (right): > > > http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc#newco... > metrics_daemon_test.cc:613: Committed_AS: 1260528 kB\n\ > Might be nice to check at least one of these numbers is being parsed > correctly, an ignored field is not being reported, and a non-ignored > field is. > > > http://codereview.chromium.org/6804014/ >
It's part of libbase or libchrome... look at $SYSROOT/usr/include/base/string_util.h. I'm not sure that there's other documentation. But there should be usage examples in the chrome source. -Ken On Fri, Apr 8, 2011 at 5:00 PM, Luigi Semenzato <semenzato@chromium.org> wrote: > Thanks! Where are documentation and header files for the string_util.h you > mention? > > On Fri, Apr 8, 2011 at 4:04 PM, <kmixter@chromium.org> wrote: >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc >> File metrics_daemon.cc (right): >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode641 >> metrics_daemon.cc:641: int file = HANDLE_EINTR(open(meminfo_path, >> O_RDONLY)); >> Consider simplifying this by just using file_util::ReadFileToString - >> though it doesn't give you the ability to truncate the file at 4K. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode662 >> metrics_daemon.cc:662: /* This array has one string for every item of >> /proc/meminfo that we want to >> Local style is to use // here and below. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode671 >> metrics_daemon.cc:671: { "MemTotal", "MemTotal" }, // SPECIAL CASE: >> total system memory >> This change increases our current number of histograms by a large %. >> Note that you'll need to add all of these to internal chrome source's >> xml separately. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode675 >> metrics_daemon.cc:675: // { "SwapCached", "SwapCached" }, >> Seems like if you're going to add spaces for alignment (which is not >> commonly done) you should make sure the commented out lines also line >> up. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode691 >> metrics_daemon.cc:691: { "Slab", "Slab", 1 }, >> Suggest either getting rid of spaces for alignment or making the >> log_scale columns align too. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode695 >> metrics_daemon.cc:695: const int nfields = sizeof(fields) / >> sizeof(fields[0]); >> arraysize(fields) >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode711 >> metrics_daemon.cc:711: LOG(WARNING) << "cannot find field " << >> fields[i].match << >> with LOG statements, the style is to have wrapped lines start with << >> which align with the << of the first line. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode720 >> metrics_daemon.cc:720: p += strlen(fields[i].match); >> I would suggest finding a way to replace all this pointer/parsing stuff >> with SplitString('\n') and SplitString(' ') or Tokenize from >> string_util.h. Less error prone and easier to maintain. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode728 >> metrics_daemon.cc:728: meminfo_value = (int) strtol(p, &rest, 10); >> static_cast >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 >> metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * >> 1000 * 1000, 100); >> This change increases our current number of histograms by a large %. >> Note that you'll need to add all of these to internal chrome source's >> xml separately. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 >> metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * >> 1000 * 1000, 100); >> So, doesn't this create a histogram with 4 million separately countable >> buckets. Wouldn't this change essentially up the memory usage by >> 4M*4B*13=208MB ironically to monitor memory usage? :) >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode746 >> metrics_daemon.cc:746: int percent = meminfo_value * 100 / total_memory; >> this will crash if the kernel ever doesn't report meminfo as the first >> line. unlikely, but probably worth a safeguard. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode786 >> metrics_daemon.cc:786: int max, int nbuckets) { >> I suggest not passing nbuckets since it's not used and might mislead >> someone to think it's only using memory proportional to nbuckets. >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode792 >> metrics_daemon.cc:792: metrics_lib_->SendEnumToUMA(name, sample, max); >> Ignoring the memory usage issue, I thought somewhere you had to document >> what each value of the "enum" meant for server display. >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h >> File metrics_daemon.h (right): >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.h#newcode65 >> metrics_daemon.h:65: FRIEND_TEST(MetricsDaemonTest, ProcessMeminfo); >> abc order >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc >> File metrics_daemon_test.cc (right): >> >> >> http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc#newco... >> metrics_daemon_test.cc:613: Committed_AS: 1260528 kB\n\ >> Might be nice to check at least one of these numbers is being parsed >> correctly, an ignored field is not being reported, and a non-ignored >> field is. >> >> http://codereview.chromium.org/6804014/ > >
Thanks Ken. Addressed all issues except one minor push back on one test. PTAL. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode641 metrics_daemon.cc:641: int file = HANDLE_EINTR(open(meminfo_path, O_RDONLY)); On 2011/04/08 23:04:05, kmixter1 wrote: > Consider simplifying this by just using file_util::ReadFileToString - though it > doesn't give you the ability to truncate the file at 4K. That's good, thanks. Output of /proc entries is limited to 4k anyway. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode662 metrics_daemon.cc:662: /* This array has one string for every item of /proc/meminfo that we want to On 2011/04/08 23:04:05, kmixter1 wrote: > Local style is to use // here and below. Cool, thanks. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode671 metrics_daemon.cc:671: { "MemTotal", "MemTotal" }, // SPECIAL CASE: total system memory On 2011/04/08 23:04:05, kmixter1 wrote: > This change increases our current number of histograms by a large %. Note that > you'll need to add all of these to internal chrome source's xml separately. Yes---separate CL (different repo) http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode675 metrics_daemon.cc:675: // { "SwapCached", "SwapCached" }, On 2011/04/08 23:04:05, kmixter1 wrote: > Seems like if you're going to add spaces for alignment (which is not commonly > done) you should make sure the commented out lines also line up. Off with the spaces! http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode691 metrics_daemon.cc:691: { "Slab", "Slab", 1 }, On 2011/04/08 23:04:05, kmixter1 wrote: > Suggest either getting rid of spaces for alignment or making the log_scale > columns align too. Getting rid! http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode695 metrics_daemon.cc:695: const int nfields = sizeof(fields) / sizeof(fields[0]); On 2011/04/08 23:04:05, kmixter1 wrote: > arraysize(fields) Yes! http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode711 metrics_daemon.cc:711: LOG(WARNING) << "cannot find field " << fields[i].match << On 2011/04/08 23:04:05, kmixter1 wrote: > with LOG statements, the style is to have wrapped lines start with << which > align with the << of the first line. Yes! http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode720 metrics_daemon.cc:720: p += strlen(fields[i].match); Yes! On 2011/04/08 23:04:05, kmixter1 wrote: > I would suggest finding a way to replace all this pointer/parsing stuff with > SplitString('\n') and SplitString(' ') or Tokenize from string_util.h. Less > error prone and easier to maintain. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100); Thanks, yes, I have gone through that a couple of times already. On 2011/04/08 23:04:05, kmixter1 wrote: > This change increases our current number of histograms by a large %. Note that > you'll need to add all of these to internal chrome source's xml separately. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100); Yes that would be ironic. But no, the number of buckets is 100, the last argument. You may be thinking of SendLinearMetric. On 2011/04/08 23:04:05, kmixter1 wrote: > So, doesn't this create a histogram with 4 million separately countable buckets. > Wouldn't this change essentially up the memory usage by 4M*4B*13=208MB > ironically to monitor memory usage? :) http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode746 metrics_daemon.cc:746: int percent = meminfo_value * 100 / total_memory; Actually, it won't, because it won't get there if it can't parse the total memory in the first line. I will make that clearer. On 2011/04/08 23:04:05, kmixter1 wrote: > this will crash if the kernel ever doesn't report meminfo as the first line. > unlikely, but probably worth a safeguard. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode786 metrics_daemon.cc:786: int max, int nbuckets) { On 2011/04/08 23:04:05, kmixter1 wrote: > I suggest not passing nbuckets since it's not used and might mislead someone to > think it's only using memory proportional to nbuckets. nbuckets is actually used. There is currently a constraint (nbuckets == max + 1) but I have a TODO there to remove it. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode792 metrics_daemon.cc:792: metrics_lib_->SendEnumToUMA(name, sample, max); On 2011/04/08 23:04:05, kmixter1 wrote: > Ignoring the memory usage issue, I thought somewhere you had to document what > each value of the "enum" meant for server display. That's because "enum" is commonly used to send enumerated types, but it's really just a linear histogram. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon_test.cc#newco... metrics_daemon_test.cc:613: Committed_AS: 1260528 kB\n\ On 2011/04/08 23:04:05, kmixter1 wrote: > Might be nice to check at least one of these numbers is being parsed correctly, > an ignored field is not being reported, and a non-ignored field is. I added a check that one number is parsed correctly. That also shows that a non-ignored field is being reported. I am pushing back on the "ignored field not being reported" because there are a lot of things that are not happening and proving that something is not happening is sometimes hard and not that useful.
LGTM http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc#newcode743 metrics_daemon.cc:743: SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100); On 2011/04/11 16:28:26, Luigi Semenzato wrote: > Yes that would be ironic. > > But no, the number of buckets is 100, the last argument. You may be thinking of > SendLinearMetric. > > > On 2011/04/08 23:04:05, kmixter1 wrote: > > So, doesn't this create a histogram with 4 million separately countable > buckets. > > Wouldn't this change essentially up the memory usage by 4M*4B*13=208MB > > ironically to monitor memory usage? :) > Apologies - I misread the code. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode681 metrics_daemon.cc:681: { "InactiveFile", "Inactive(file)" }, why is it a no-no? http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode732 metrics_daemon.cc:732: LOG(WARNING) << "missing meminfo value"; total_memory could have been reported as 0 and reach here. I don't know why it would, but same suggestion to avoid div/0. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode767 metrics_daemon.cc:767: if (count <= 0) Prefer // here. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode769 metrics_daemon.cc:769: Ah, I understand. Fair enough - you're forcing the caller to know how many buckets are required. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:597: SwapTotal: 0 kB\n\ Seems like you could add a line like: EXPECT_CALL(metrics_lib_, SendToUMA("SwapTotal", _, _, _, _, _).Times(AtLeast(0)).Invoke(FailFunction); To make sure SendToUMA is never called with that pattern. I guess you'd also have to do for SendEnumToUMA as well. I'll grant you this doesn't test every possible way it could be reported, but it does make an effort. I would personally do this, but test-worthiness can be subjective. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:620: EXPECT_CALL(metrics_lib_, SendEnumToUMA(_, _, 100)) Did you mention you added a test that at least one number was parsed ok? I don't see it.
Thanks, applied final suggestions and pushed. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc File metrics_daemon.cc (right): http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode681 metrics_daemon.cc:681: const int nfields = sizeof(fields) / sizeof(fields[0]); // arraysize no-no On 2011/04/12 19:04:33, kmixter1 wrote: > why is it a no-no? I don't know. Maybe it doesn't like anonymous structures. This is what I get when I use "arraysize(fields)": metrics_daemon.cc: In member function 'gboolean MetricsDaemon::ProcessMeminfo(std::string)': metrics_daemon.cc:682: error: no matching function for call to 'ArraySizeHelper(MetricsDaemon::ProcessMeminfo(std::string)::<anonymous struct> [15])' make: *** [metrics_daemon.o] Error 1 http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode732 metrics_daemon.cc:732: int percent = meminfo_value * 100 / total_memory; On 2011/04/12 19:04:33, kmixter1 wrote: > total_memory could have been reported as 0 and reach here. I don't know why it > would, but same suggestion to avoid div/0. Yes. So now we can't reach this statement when total_memory == 0. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon.cc#newcode767 metrics_daemon.cc:767: /* TODO(semenzato): add a proper linear histogram to the Chrome external On 2011/04/12 19:04:33, kmixter1 wrote: > Prefer // here. Thanks, missed that one. http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc File metrics_daemon_test.cc (right): http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:597: SwapTotal: 0 kB\n\ On 2011/04/12 19:04:33, kmixter1 wrote: > Seems like you could add a line like: > > EXPECT_CALL(metrics_lib_, SendToUMA("SwapTotal", _, _, _, _, > _).Times(AtLeast(0)).Invoke(FailFunction); > > To make sure SendToUMA is never called with that pattern. I guess you'd also > have to do for SendEnumToUMA as well. I'll grant you this doesn't test every > possible way it could be reported, but it does make an effort. I would > personally do this, but test-worthiness can be subjective. Oh I see, thanks. But I can't find the FailFunction you mention, so I am just making it .Times(0). http://codereview.chromium.org/6804014/diff/11001/metrics_daemon_test.cc#newc... metrics_daemon_test.cc:620: EXPECT_CALL(metrics_lib_, SendEnumToUMA(_, _, 100)) On 2011/04/12 19:04:33, kmixter1 wrote: > Did you mention you added a test that at least one number was parsed ok? I > don't see it. It's below, in ProcessMeminfo2. It reports one stat before it fails. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
