Report histogram creation results.

chrome/installer/setup/installer_metrics.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/installer/setup/installer_metrics.h"
 
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_persistence.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "chrome/installer/util/util_constants.h"
 
 namespace installer {
 
 void BeginPersistentHistogramStorage() {
   base::SetPersistentHistogramMemoryAllocator(
       new base::LocalPersistentMemoryAllocator(
           1 << 20, 0,  // 1 MiB
           installer::kSetupHistogramAllocatorName));
   base::GetPersistentHistogramMemoryAllocator()->CreateTrackingHistograms(
       installer::kSetupHistogramAllocatorName);
+
+  // This can't be enabled until after the allocator is configured because
+  // there is no other reporting out of setup other than persistent memory.
+  base::HistogramBase::EnableCreationReportHistogram("setup");

[Alexei Svitkine (slow), 2016/03/01 16:41:34]

I find it unfortunate that we need an extra call (and an extra string param). Is
there any way to make the SetPersistentHistogramMemoryAllocator() call do this
internally w/ the allocator name string that's passed? Maybe there could be a
param to it that specifies whether it should do this or not - if we don't always
want it.

A second question - why do we need it be a separate call to enable the histogram
and to cache the histogram object? As opposed to e.g. just using a standard
macro and making the code early return if the name hasn't been set?

Is it just for testing? If so, you can use HistogramTester class and not need to
actually access the histogram object.

[bcwhite, 2016/03/02 19:14:19]

Most of the time, the Report histogram will be enabled first.  Also, I don't
think it is something that histogram_persistence should know about -- it's a
completely orthogonal thing.
This particular histogram doesn't get a single update per activity.  The
creation action, for example, could cause increments in up to four different
buckets: total count, the type, uma stability|targeted, and is-persistent.  A
macro instantiation can't do that efficiently.
It's for reporting.  I wanted to see what and how histograms were being used in
the field.  It already found one issue which led to a 40% time reduction in
creating new histograms.

[Alexei Svitkine (slow), 2016/03/03 17:56:23]

You can have a helper function that calls the macro and call it with different
enum values. It should be pretty efficient - only overhead is the function call
per value logged, but that would be negligible.
Sorry, I understand that you want the histogram in the wild. My question is why
you're constructing it manually and keeping a pointer to it? (As opposed to
using a standard UMA_ macro guarded by a check if the process name has been
set.) If you're only using the pointer to access it from tests, there's a
HistogramTester class that can be used that doesn't require it.

[bcwhite, 2016/03/07 13:39:25]

I guess I've done so much work on histograms that using FactoryGet seems
natural.  Given that this call would be done while single-threaded, it has no
need for atomics and conditional creation.

It also avoids saving an extra copy of the string.  The std::string copy held
inside the histogram may be copy-on-write but persistent histograms by necessity
keep a separate copy of the histogram name in persistent space; once the
transition to them is done, I hope to eliminate the extra std::string copy of
the name and change FactoryGet to take a StringPiece instead.

But as you say, the overhead of the multiple subroutine calls, atomic reads,
conditions, and returns is pretty small and applies only to the creation of all
histograms, no their use.  If you'd prefer it done that way, I'll change it.

[Alexei Svitkine (slow), 2016/03/07 16:16:46]

Right - my preference is to use the macros because does make the code less
complex and is easier to understand.

[bcwhite, 2016/03/07 20:09:25]

Well...  We're just going to have to agree to disagree on that particular point.
 :-)  But I did try.  It adds complications to avoid recursive calls to the
macro and requires an independent unit-test that doesn't try to create a
StatisticsRecorder.

I had to back it out, though, because it breaks other tests.

The basic problem is that HistogramTester objects do not play nice with local
StastiticsRecorder objects.

Specifically, HistogramTester calls StatisticsRecorder::Initialize() which
creates the global SR object.  After that point, any attempt to create a local
SR object will crash on DCHECK(!histograms_) in the SR constructor.

base_unittests has one existing instance of HistogramTester and that is within
histogram_tester_unittest.cc.  By luck, this test conveniently runs later and
only one test after it attempts to create local SR objects.  The only one that
does is...  (drum roll, please) "HistogramTest" ...which as you may recall
included the SR::ResetForTesting() method (that I now see is pretty much a hack
to get around the just-described problem).

The fix for this is non-trivial.  If HistogramTester were to create its own
instance of a StatisticsRecorder (which was obviously avoided purposely during
its development) then it would crash if any previous test initialized the
lazy-instance with the (global) SR.  The simplest solution seems to be to simply
delete the global instance inside ResetForTesting()...  except there is no way
to reset a LazyInstance.

Regardless of if/when/how this gets fixed, it doesn't belong in this CL so I've
gone back to my original solution.  I can revisit this if you feel it necessary
once the larger problem has been resolved.  I'll add that to my list of things
to do when bored.  ;-)

Incidentally, the reason the ResetForTesting() hack doesn't work here when it
worked elsewhere is, again, test ordering.  Because tests that run later that
HistogramBaseTest depend on ::Initialize() to create a working SR but that
method won't do so if it did so previously but was then reset.  The result is
that histograms don't get recorded by the SR and tests fail.

[Alexei Svitkine (slow), 2016/03/07 20:26:22]

Okay, that's fair. Can you add a comment about this to the callsite where you
use the Factory method about this reasoning?

However, if we have an existing issue that depends on test ordering, we need to
fix it. I tried running base_unittests a few times with --gunit_shuffle and
didn't see any failures. Are you saying there are orderings that it does fail
in?

[bcwhite, 2016/03/07 20:47:34]

Done.
Try adding "--single-process-tests" to ensure that everything runs on a single
thread.  With that and --gtest_shuffle, I got the DCHECK first try.

[Alexei Svitkine (slow), 2016/03/07 21:14:50]

Yep - I see it! (I was using --gunit_shuffle instead of --gtest_shuffle - oops.)

Can you file a bug for it? Thanks!

[bcwhite, 2016/03/07 22:20:41]

Done.

 }
 
 void EndPersistentHistogramStorage(const base::FilePath& target_path) {
   // For atomicity, first write to a temporary file and then rename it.
   // The ImportantFileWriter would be good for this except it supports only
   // std::string for its data.
   base::PersistentMemoryAllocator* allocator =
       base::GetPersistentHistogramMemoryAllocator();
   allocator->UpdateTrackingHistograms();
 
@@ skipping 12 matching lines @@
       if (base::ReplaceFile(tmp_file_path, file_path, nullptr)) {
         VLOG(1) << "Persistent histograms saved in file: "
                 << file_path.value();
       }
     }
     base::DeleteFile(tmp_file_path, false);
   }
 }
 
 }  // namespace installer