Reland "Add a HistogramRecorder class and use cases."

Reland "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.

This reverts commit b957026f43ef9464a8cdd5a240867c807b2281da.

BUG=232414
TBR=rsleevi, mark, groby, thakis

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242121

[Reid Kleckner, 2013-07-29 16:51:34]

This error from URLRequestThrottlerSimulation looks relevant:

InvalidRead
Invalid read of size 4
  disk_cache::Stats::Snapshot(base::HistogramSamples*) const
(net/disk_cache/stats.cc:250)
  disk_cache::StatsHistogram::SnapshotSamples() const
(net/disk_cache/stats_histogram.cc:75)
  base::HistogramRecorder::HistogramRecorder()
(base/test/histogram_recorder.cc:26)
  net::URLRequestThrottlerEntryTest::SetUp()
(net/url_request/url_request_throttler_unittest.cc:197)
Address 0xd16b03c is 20 bytes before a block of size 32 free'd
  operator delete(void*) (m_replacemalloc/vg_replace_malloc.c:1149)
  __gnu_cxx::new_allocator<int>::deallocate(int*, unsigned long)
(/usr/include/c++/4.6/ext/new_allocator.h:98)
  std::_Vector_base<int, std::allocator<int> >::_M_deallocate(int*, unsigned
long) (/usr/include/c++/4.6/bits/stl_vector.h:156)
  std::vector<int, std::allocator<int>
>::_M_insert_aux(__gnu_cxx::__normal_iterator<int*, std::vector<int,
std::allocator<int> > >, int const&) (/usr/include/c++/4.6/bits/vector.tcc:366)
  std::vector<int, std::allocator<int> >::push_back(int const&)
(/usr/include/c++/4.6/bits/stl_vector.h:834)
  base::CustomHistogram::ArrayToCustomRanges(int const*, unsigned long)
(base/metrics/histogram.cc:738)
  net::GetAllErrorCodesForUma() (net/base/net_errors.cc:44)
 
net::ClientSocketPoolHistograms::ClientSocketPoolHistograms(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&)
(net/socket/client_socket_pool_histograms.cc:51)
  net::ClientSocketPoolManagerImpl::ClientSocketPoolManagerImpl(net::NetLog*,
net::ClientSocketFactory*, net::HostResolver*, net::CertVerifier*,
net::ServerBoundCertService*, net::TransportSecurityState*,
std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&,
net::ProxyService*, net::SSLConfigService*,
net::HttpNetworkSession::SocketPoolType)
(net/socket/client_socket_pool_manager_impl.cc:85)
  (anonymous
namespace)::CreateSocketPoolManager(net::HttpNetworkSession::SocketPoolType,
net::HttpNetworkSession::Params const&) (net/http/http_network_session.cc:48)
  net::HttpNetworkSession::HttpNetworkSession(net::HttpNetworkSession::Params
const&) (net/http/http_network_session.cc:131)
  net::TestURLRequestContext::Init()
(net/url_request/url_request_test_util.cc:108)
  net::TestURLRequestContext::TestURLRequestContext()
(net/url_request/url_request_test_util.cc:50)
  net::(anonymous namespace)::Server::Server(int, double)
(net/url_request/url_request_throttler_simulation_unittest.cc:127)
  net::(anonymous namespace)::SimulateDowntime(base::TimeDelta const&,
base::TimeDelta const&, bool)
(net/url_request/url_request_throttler_simulation_unittest.cc:597)
  net::(anonymous
namespace)::URLRequestThrottlerSimulation_PerceivedDowntimeRatio_Test::TestBody()
(net/url_request/url_request_throttler_simulation_unittest.cc:720)

Try running that test suite by itself.  We have instructions on how to run
valgrind locally:
http://www.chromium.org/developers/how-tos/using-valgrind

Hopefully that helps.  If it's confusing, feel free to email
chrome-valgrind-team@google.com for help.

[lpromero, 2013-07-29 16:57:12]

On 2013/07/29 16:51:34, Reid Kleckner wrote:
> This error from URLRequestThrottlerSimulation looks relevant:
> 
> InvalidRead
> Invalid read of size 4
>   disk_cache::Stats::Snapshot(base::HistogramSamples*) const
> (net/disk_cache/stats.cc:250)
>   disk_cache::StatsHistogram::SnapshotSamples() const
> (net/disk_cache/stats_histogram.cc:75)
>   base::HistogramRecorder::HistogramRecorder()
> (base/test/histogram_recorder.cc:26)
>   net::URLRequestThrottlerEntryTest::SetUp()
> (net/url_request/url_request_throttler_unittest.cc:197)
> Address 0xd16b03c is 20 bytes before a block of size 32 free'd
>   operator delete(void*) (m_replacemalloc/vg_replace_malloc.c:1149)
>   __gnu_cxx::new_allocator<int>::deallocate(int*, unsigned long)
> (/usr/include/c++/4.6/ext/new_allocator.h:98)
>   std::_Vector_base<int, std::allocator<int> >::_M_deallocate(int*, unsigned
> long) (/usr/include/c++/4.6/bits/stl_vector.h:156)
>   std::vector<int, std::allocator<int>
> >::_M_insert_aux(__gnu_cxx::__normal_iterator<int*, std::vector<int,
> std::allocator<int> > >, int const&)
(/usr/include/c++/4.6/bits/vector.tcc:366)
>   std::vector<int, std::allocator<int> >::push_back(int const&)
> (/usr/include/c++/4.6/bits/stl_vector.h:834)
>   base::CustomHistogram::ArrayToCustomRanges(int const*, unsigned long)
> (base/metrics/histogram.cc:738)
>   net::GetAllErrorCodesForUma() (net/base/net_errors.cc:44)
>  
>
net::ClientSocketPoolHistograms::ClientSocketPoolHistograms(std::basic_string<char,
> std::char_traits<char>, std::allocator<char> > const&)
> (net/socket/client_socket_pool_histograms.cc:51)
>   net::ClientSocketPoolManagerImpl::ClientSocketPoolManagerImpl(net::NetLog*,
> net::ClientSocketFactory*, net::HostResolver*, net::CertVerifier*,
> net::ServerBoundCertService*, net::TransportSecurityState*,
> std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&,
> net::ProxyService*, net::SSLConfigService*,
> net::HttpNetworkSession::SocketPoolType)
> (net/socket/client_socket_pool_manager_impl.cc:85)
>   (anonymous
> namespace)::CreateSocketPoolManager(net::HttpNetworkSession::SocketPoolType,
> net::HttpNetworkSession::Params const&) (net/http/http_network_session.cc:48)
>   net::HttpNetworkSession::HttpNetworkSession(net::HttpNetworkSession::Params
> const&) (net/http/http_network_session.cc:131)
>   net::TestURLRequestContext::Init()
> (net/url_request/url_request_test_util.cc:108)
>   net::TestURLRequestContext::TestURLRequestContext()
> (net/url_request/url_request_test_util.cc:50)
>   net::(anonymous namespace)::Server::Server(int, double)
> (net/url_request/url_request_throttler_simulation_unittest.cc:127)
>   net::(anonymous namespace)::SimulateDowntime(base::TimeDelta const&,
> base::TimeDelta const&, bool)
> (net/url_request/url_request_throttler_simulation_unittest.cc:597)
>   net::(anonymous
>
namespace)::URLRequestThrottlerSimulation_PerceivedDowntimeRatio_Test::TestBody()
> (net/url_request/url_request_throttler_simulation_unittest.cc:720)
> 
> Try running that test suite by itself.  We have instructions on how to run
> valgrind locally:
> http://www.chromium.org/developers/how-tos/using-valgrind
> 
> Hopefully that helps.  If it's confusing, feel free to email
> mailto:chrome-valgrind-team@google.com for help.

Thanks for the help! I'm going to investigate this.

[commit-bot: I haz the power, 2013-12-17 13:24:03]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[lpromero, 2013-12-17 13:54:02]

I'd like to reland this. Could you review it?

Thank you.

[Reid Kleckner, 2013-12-17 18:46:13]

I can't actually review the patch content, I'm not familiar with the code.  I'm
just looking to see if the memory error (which I only faintly recall) was fixed.

It looks like the trybot is failing to apply the patch.  Can you rebase it and
retry it?

If the try run fails due to random bot sickness, then I'll stamp it and see if
the memory waterfall catches it.

[lpromero, 2013-12-17 18:53:15]

Hi Reid,

Sorry for the mess in the patches. I cleaned all of that. Patch 1 is a strict
revert of the revert, and patch 2 addresses the initialization problem (and a
small rebase resolution wrt StatisticsRecorder).

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
File base/test/histogram_recorder_unittest.cc (right):

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder_unittest.cc:16: HistogramRecorder::Initialize();
HistogramRecorders are now globally initialized, before any test case can run.
The problem with the original CL was that HistogramRecorder where initialized to
late. This fix could also fix crbug.com/230534, mentioned in a comment in
spellcheck_host_metrics_unittest.cc.

[ppi, 2013-12-19 16:57:26]

Thanks, Louis!

lgtm with nits (note that I am not OWNER of anything relevant).

It seems that the original CL (https://chromiumcodereview.appspot.com/18337014)
had all necessary rubber stamps, should this be TBRed to original reviewers once
Reid is fine with the patch?

https://codereview.chromium.org/19866004/diff/62001/base/metrics/statistics_r...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/19866004/diff/62001/base/metrics/statistics_r...
base/metrics/statistics_recorder.h:73: // caller supplied vector (Histograms). 
Only histograms which have query as
nit: remove double space, put "query" in pipes: |query|

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:16: // tests to check that metrics were sent as
they should be.
Should we say "recorded" instead of "sent"? Am I correct that they are always
recorded, but not always sent?

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:34: // lifecycle.
Could we clarify the ownership of HistogramSamples stored in the map in a
comment? Raw pointers seem to indicate that someone else is owning and deleting
them.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:35: std::map<std::string, HistogramSamples*>
original_samples_;
DISALLOW_COPY_AND_ASSIGN(HistogramRecorder);

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
File base/test/histogram_recorder_unittest.cc (right):

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder_unittest.cc:21: // Send a histogram before the
creation of the recorder.
Should we say "Record" instead of "Send"?

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder_unittest.cc:32: // Send a histogram after the
creation of the recorder.
Should we say "Record" instead of "Send"?

[Reid Kleckner, 2013-12-19 19:20:24]

lgtm

[lpromero, 2013-12-20 11:53:43]

https://codereview.chromium.org/19866004/diff/62001/base/metrics/statistics_r...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/19866004/diff/62001/base/metrics/statistics_r...
base/metrics/statistics_recorder.h:73: // caller supplied vector (Histograms). 
Only histograms which have query as
On 2013/12/19 16:57:27, ppi wrote:
> nit: remove double space, put "query" in pipes: |query|

Done.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:16: // tests to check that metrics were sent as
they should be.
On 2013/12/19 16:57:27, ppi wrote:
> Should we say "recorded" instead of "sent"? Am I correct that they are always
> recorded, but not always sent?

Yes, you are correct. By sent, I meant triggered from the code to the histogram
"engine", not sent from the histogram engine to our servers. I agree that
"record" is a better wording. Changing here and everywhere.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:34: // lifecycle.
On 2013/12/19 16:57:27, ppi wrote:
> Could we clarify the ownership of HistogramSamples stored in the map in a
> comment? Raw pointers seem to indicate that someone else is owning and
deleting
> them.

Done.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder.h:35: std::map<std::string, HistogramSamples*>
original_samples_;
On 2013/12/19 16:57:27, ppi wrote:
> DISALLOW_COPY_AND_ASSIGN(HistogramRecorder);

Done.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
File base/test/histogram_recorder_unittest.cc (right):

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder_unittest.cc:21: // Send a histogram before the
creation of the recorder.
On 2013/12/19 16:57:27, ppi wrote:
> Should we say "Record" instead of "Send"?

Done.

https://codereview.chromium.org/19866004/diff/62001/base/test/histogram_recor...
base/test/histogram_recorder_unittest.cc:32: // Send a histogram after the
creation of the recorder.
On 2013/12/19 16:57:27, ppi wrote:
> Should we say "Record" instead of "Send"?

Done.

[blundell, 2013-12-20 12:29:38]

LGTM

https://codereview.chromium.org/19866004/diff/82001/base/test/histogram_recor...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/82001/base/test/histogram_recor...
base/test/histogram_recorder.h:35: // lifecycle. This isntance takes ownership
of the samples, that are deleted
instance

https://codereview.chromium.org/19866004/diff/82001/base/test/histogram_recor...
base/test/histogram_recorder.h:35: // lifecycle. This isntance takes ownership
of the samples, that are deleted
s/that/which

[lpromero, 2013-12-20 12:37:10]

TBR Mark Mentovai for base.
TBR groby for spellcheck.
TBR Nico for cocoa.
TBR Ryan Sleevi for net.

Thank you.

[lpromero, 2013-12-20 12:37:50]

https://codereview.chromium.org/19866004/diff/82001/base/test/histogram_recor...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/82001/base/test/histogram_recor...
base/test/histogram_recorder.h:35: // lifecycle. This isntance takes ownership
of the samples, that are deleted
On 2013/12/20 12:29:38, blundell wrote:
> s/that/which

Done.

[commit-bot: I haz the power, 2013-12-20 12:38:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lpromero@chromium.org/19866004/112001

[Ryan Sleevi, 2013-12-20 16:52:39]

lgtm

[commit-bot: I haz the power, 2013-12-20 17:46:57]

Change committed as 242121

[Mark Mentovai, 2013-12-20 18:07:11]

This doesn’t seems so urgent that it needed to be TBRed. But the followup fixes
should be easy.

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:21: static void Initialize();
This shouldn’t precede the constructor.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:23: virtual ~HistogramRecorder();
Why is this virtual? There are no other virtual methods here. This class is not
a base of any other class. This adds unnecessary indirection.

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:37: std::map<std::string, HistogramSamples*>
original_samples_;
Include what you use: #include <string>

[lpromero, 2013-12-20 18:22:41]

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:21: static void Initialize();
On 2013/12/20 18:07:12, Mark Mentovai wrote:
> This shouldn’t precede the constructor.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:23: virtual ~HistogramRecorder();
On 2013/12/20 18:07:12, Mark Mentovai wrote:
> Why is this virtual? There are no other virtual methods here. This class is
not
> a base of any other class. This adds unnecessary indirection.

Done.

https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
base/test/histogram_recorder.h:37: std::map<std::string, HistogramSamples*>
original_samples_;
On 2013/12/20 18:07:12, Mark Mentovai wrote:
> Include what you use: #include <string>

Done.

[lpromero, 2013-12-20 18:23:04]

On 2013/12/20 18:22:41, lpromero wrote:
>
https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
> File base/test/histogram_recorder.h (right):
> 
>
https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
> base/test/histogram_recorder.h:21: static void Initialize();
> On 2013/12/20 18:07:12, Mark Mentovai wrote:
> > This shouldn’t precede the constructor.
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> Done.
> 
>
https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
> base/test/histogram_recorder.h:23: virtual ~HistogramRecorder();
> On 2013/12/20 18:07:12, Mark Mentovai wrote:
> > Why is this virtual? There are no other virtual methods here. This class is
> not
> > a base of any other class. This adds unnecessary indirection.
> 
> Done.
> 
>
https://codereview.chromium.org/19866004/diff/112001/base/test/histogram_reco...
> base/test/histogram_recorder.h:37: std::map<std::string, HistogramSamples*>
> original_samples_;
> On 2013/12/20 18:07:12, Mark Mentovai wrote:
> > Include what you use: #include <string>
> 
> Done.

The feedback is addressed in https://codereview.chromium.org/99553004/