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

Issue 18337014: Add a HistogramRecorder class and use cases. (Closed)

Created:
7 years, 5 months ago by lpromero
Modified:
7 years, 5 months ago
CC:
chromium-reviews, groby+spellwatch_chromium.org, rpetterson, erikwright+watch_chromium.org, rouslan+spellwatch_chromium.org, cbentzel+watch_chromium.org
Visibility:
Public.

Description

Add a HistogramRecorder class and use cases. This CL adds a utility class to streamline the task of testing that metrics have been updated as expected. Specifically, the HistogramRecorder class allows a client to obtain the differential value of a given histogram in the interval since the given HistogramRecorder instance was created. BUG=232414 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212378

Patch Set 1 #

Patch Set 2 : Clean up. #

Patch Set 3 : Remove an empty line. #

Total comments: 4

Patch Set 4 : Review feedback. #

Patch Set 5 : Add a test class. #

Patch Set 6 : Refine a comment. #

Patch Set 7 : Return scoped_ptr-s. #

Patch Set 8 : Remove the query parameter. #

Patch Set 9 : Remove explicit keyword. #

Total comments: 9

Patch Set 10 : Nico feedback. #

Patch Set 11 : scoped_ptr sanitization. #

Patch Set 12 : Synced. #

Total comments: 4

Patch Set 13 : Remove .get(). #

Patch Set 14 : Synced. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -164 lines) Patch
M base/base.gyp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
M base/metrics/statistics_recorder.h View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
A base/test/histogram_recorder.h View 1 2 3 4 5 6 7 8 1 chunk +35 lines, -0 lines 0 comments Download
A base/test/histogram_recorder.cc View 1 2 3 4 5 6 7 11 12 1 chunk +42 lines, -0 lines 0 comments Download
A base/test/histogram_recorder_unittest.cc View 1 2 3 4 5 6 7 11 12 1 chunk +34 lines, -0 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +31 lines, -67 lines 0 comments Download
M chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +7 lines, -30 lines 0 comments Download
M net/url_request/url_request_throttler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +15 lines, -64 lines 1 comment Download

Messages

Total messages: 20 (0 generated)
lpromero
7 years, 5 months ago (2013-07-10 08:34:59 UTC) #1
blundell
How hard would it be to write a unit test for the new class? https://codereview.chromium.org/18337014/diff/3001/base/test/histogram_recorder.h ...
7 years, 5 months ago (2013-07-10 14:35:36 UTC) #2
lpromero
Working on the test. https://codereview.chromium.org/18337014/diff/3001/base/test/histogram_recorder.h File base/test/histogram_recorder.h (right): https://codereview.chromium.org/18337014/diff/3001/base/test/histogram_recorder.h#newcode18 base/test/histogram_recorder.h:18: // Constructor takes a histogram ...
7 years, 5 months ago (2013-07-10 17:08:32 UTC) #3
lpromero
PTAL Refactored the recorder to not use a query string. The API is cleaner and ...
7 years, 5 months ago (2013-07-12 11:31:53 UTC) #4
blundell
lgtm +mark for base +wtc for net +groby for spellcheck +thakis for cocoa https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc File ...
7 years, 5 months ago (2013-07-12 13:22:42 UTC) #5
Mark Mentovai
LGTM in base.
7 years, 5 months ago (2013-07-12 14:59:42 UTC) #6
groby-ooo-7-16
spellcheck lgtm You're probably aware, but there are other tests that use the baseline/subtract approach. ...
7 years, 5 months ago (2013-07-12 19:13:48 UTC) #7
Nico
cocoa lgtm https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.h File base/test/histogram_recorder.h (right): https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.h#newcode24 base/test/histogram_recorder.h:24: scoped_ptr<HistogramSamples> GetHistogramSamplesSinceCreation( nit: I'd give a function ...
7 years, 5 months ago (2013-07-15 17:38:27 UTC) #8
lpromero
On 2013/07/12 19:13:48, groby wrote: > spellcheck lgtm > > You're probably aware, but there ...
7 years, 5 months ago (2013-07-17 13:40:28 UTC) #9
lpromero
Thanks for the review! https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc File base/test/histogram_recorder.cc (right): https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc#newcode38 base/test/histogram_recorder.cc:38: named_samples->Subtract(*named_original_samples); On 2013/07/12 13:22:42, blundell ...
7 years, 5 months ago (2013-07-17 13:40:47 UTC) #10
blundell
-wtc, +sleevi for net/, as I think wtc is OOO on vacation.
7 years, 5 months ago (2013-07-17 16:14:40 UTC) #11
Ryan Sleevi
LGTM with nits. https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc File base/test/histogram_recorder.cc (right): https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc#newcode38 base/test/histogram_recorder.cc:38: named_samples->Subtract(*named_original_samples); On 2013/07/12 13:22:42, blundell wrote: ...
7 years, 5 months ago (2013-07-17 17:03:26 UTC) #12
lpromero
https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc File base/test/histogram_recorder.cc (right): https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc#newcode38 base/test/histogram_recorder.cc:38: named_samples->Subtract(*named_original_samples); On 2013/07/17 17:03:26, Ryan Sleevi wrote: > On ...
7 years, 5 months ago (2013-07-18 07:43:37 UTC) #13
blundell
https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc File base/test/histogram_recorder.cc (right): https://codereview.chromium.org/18337014/diff/39001/base/test/histogram_recorder.cc#newcode38 base/test/histogram_recorder.cc:38: named_samples->Subtract(*named_original_samples); Oops, I had misunderstood what was going on ...
7 years, 5 months ago (2013-07-18 07:43:53 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lpromero@chromium.org/18337014/72009
7 years, 5 months ago (2013-07-18 12:15:37 UTC) #15
commit-bot: I haz the power
Change committed as 212378
7 years, 5 months ago (2013-07-18 18:51:02 UTC) #16
wtc
https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_request_throttler_unittest.cc File net/url_request/url_request_throttler_unittest.cc (right): https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_request_throttler_unittest.cc#newcode1 net/url_request/url_request_throttler_unittest.cc:1: Nit: remove this blank line.
7 years, 5 months ago (2013-07-26 22:11:13 UTC) #17
lpromero
On 2013/07/26 22:11:13, wtc wrote: > https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_request_throttler_unittest.cc > File net/url_request/url_request_throttler_unittest.cc (right): > > https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_request_throttler_unittest.cc#newcode1 > ...
7 years, 5 months ago (2013-07-26 22:12:24 UTC) #18
lpromero
On 2013/07/26 22:12:24, lpromero wrote: > On 2013/07/26 22:11:13, wtc wrote: > > > https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_request_throttler_unittest.cc ...
7 years, 5 months ago (2013-07-26 22:12:51 UTC) #19
lpromero
7 years, 5 months ago (2013-07-26 22:12:52 UTC) #20
Message was sent while issue was closed.
On 2013/07/26 22:12:24, lpromero wrote:
> On 2013/07/26 22:11:13, wtc wrote:
> >
>
https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_reque...
> > File net/url_request/url_request_throttler_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/18337014/diff/72009/net/url_request/url_reque...
> > net/url_request/url_request_throttler_unittest.cc:1: 
> > 
> > Nit: remove this blank line.
> 
> This CL has been reverted and in the reland, I spotted this line. It will be
> removed in the reland. Thanks!

Reland will take place here: https://codereview.chromium.org/19866004/

Powered by Google App Engine
This is Rietveld 408576698