arc: Handle metrics for Auth operations.

chrome/browser/chromeos/arc/arc_optin_uma.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/browser/chromeos/arc/arc_optin_uma.h"
 
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
 
@@ skipping 33 matching lines @@
   // The macro UMA_HISTOGRAM_CUSTOM_TIMES expects a constant string, but since
   // this measurement happens very infrequently, we do not need to use a macro
   // here.
   base::Histogram::FactoryTimeGet(
       histogram_name, base::TimeDelta::FromSeconds(1),
       base::TimeDelta::FromMinutes(6), 50,
       base::HistogramBase::kUmaTargetedHistogramFlag)
       ->AddTime(elapsed_time);
 }
 
+void UpdateAuthTiming(const char* histogram_name,
+                      base::TimeDelta elapsed_time) {
+  base::Histogram::FactoryTimeGet(
+      histogram_name, base::TimeDelta::FromSeconds(1) /* minimum */,
+      base::TimeDelta::FromMinutes(3) /* maximum */, 50 /* bucket_count */,
+      base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->AddTime(elapsed_time);

[Ilya Sherman, 2017/02/23 21:23:34]

Please use the convenience wrapper base::UmaHistogramCustomTimes from
histogram_functions.h.

[khmel, 2017/02/23 21:57:45]

Done.

+}
+
+void UpdateAuthAttempts(const char* histogram_name, int32_t value) {
+  UMA_HISTOGRAM_CUSTOM_COUNTS(histogram_name, value, 0 /* minimum */,
+                              5 /* maximum */, 6 /* bucket_count */);

[Ilya Sherman, 2017/02/23 21:23:34]

Please use the convenience wrapper base::UmaHistogramExactLinear or
UMA_HISTOGRAM_SPARSE_SLOWLY if you need to record to multiple named histograms. 
Note that it's not safe to pass a runtime-variable name to most of the
UMA_HISTOGRAM macros (the one exception being UMA_HISTOGRAM_SPARSE_SLOWLY).

However, it looks like you are actually only ever passing a single histogram
name to this function.  Why pass in the name as a parameter at all?  I'd
recommend writing this function as

void UpdateAuthAttempts(int32_t num_attempts) {
  UMA_HISTOGRAM_SPARSE_SLOWLY("ArcAuth.CheckinAttempts", num_attempts);
}

(Note that I also renamed the generically named "value" to the clearer
"num_attempts".)

[khmel, 2017/02/23 21:57:45]

Thanks for detailed comment. I reserved it for possible extension and to keep it
aligned with prev function. I am ok to use name here.
Done

+}
+
 void UpdateSilentAuthCodeUMA(OptInSilentAuthCode state) {
   UMA_HISTOGRAM_ENUMERATION("Arc.OptInSilentAuthCode", static_cast<int>(state),
                             static_cast<int>(OptInSilentAuthCode::SIZE));
 }
 
 std::ostream& operator<<(std::ostream& os, const ProvisioningResult& result) {
 #define MAP_PROVISIONING_RESULT(name) \
   case ProvisioningResult::name:      \
     return os << #name
 
@@ skipping 22 matching lines @@
 
 #undef MAP_PROVISIONING_RESULT
 
   // Some compilers report an error even if all values of an enum-class are
   // covered exhaustively in a switch statement.
   NOTREACHED() << "Invalid value " << static_cast<int>(result);
   return os;
 }
 
 }  // namespace arc