Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(222)

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.

Description

Add 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -0 lines) Patch
M metrics_daemon.h View 1 2 3 4 4 chunks +23 lines, -0 lines 0 comments Download
M metrics_daemon.cc View 1 2 3 4 5 chunks +134 lines, -0 lines 0 comments Download
M metrics_daemon_test.cc View 1 2 3 4 2 chunks +61 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Luigi Semenzato
Darin please review code. Mandeep and Stephan please comment on numbers collected and frequency of ...
9 years, 8 months ago (2011-04-06 17:29:07 UTC) #1
Mandeep Singh Baines
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: // ...
9 years, 8 months ago (2011-04-06 22:35:26 UTC) #2
Luigi Semenzato
Added and tested, PTAL. On 2011/04/06 22:35:26, Mandeep Singh Baines wrote: > Maybe add a ...
9 years, 8 months ago (2011-04-06 23:04:41 UTC) #3
Mandeep Singh Baines
LGTM semenzato@chromium.org (semenzato@chromium.org) wrote: > Added and tested, PTAL. > > On 2011/04/06 22:35:26, Mandeep ...
9 years, 8 months ago (2011-04-06 23:34:54 UTC) #4
Luigi Semenzato
Hi Ken. Ping? Thanks! On 2011/04/06 23:34:54, Mandeep Singh Baines wrote: > LGTM > > ...
9 years, 8 months ago (2011-04-08 22:23:04 UTC) #5
kmixter1
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 ...
9 years, 8 months ago (2011-04-08 23:04:05 UTC) #6
Luigi Semenzato
Thanks! Where are documentation and header files for the string_util.h you mention? On Fri, Apr ...
9 years, 8 months ago (2011-04-09 00:00:14 UTC) #7
kmixter1
It's part of libbase or libchrome... look at $SYSROOT/usr/include/base/string_util.h. I'm not sure that there's other ...
9 years, 8 months ago (2011-04-09 00:14:56 UTC) #8
Luigi Semenzato
Thanks Ken. Addressed all issues except one minor push back on one test. PTAL. http://codereview.chromium.org/6804014/diff/4001/metrics_daemon.cc ...
9 years, 8 months ago (2011-04-11 16:28:26 UTC) #9
kmixter1
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, ...
9 years, 8 months ago (2011-04-12 19:04:33 UTC) #10
Luigi Semenzato
9 years, 8 months ago (2011-04-12 21:12:32 UTC) #11
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.

Powered by Google App Engine
This is Rietveld 408576698