[3793] Changes after review:

 - {ptime,duration}ToText moved to lib/util
 - missing comments added
 - EXPECT_EQ -> ASSERT_EQ
 - several consts added
 - protected methods are now private in Observation
 - tabs converted to spaces

configure.ac

@@ -1494,8 +1494,8 @@ AC_CONFIG_FILES([compatcheck/Makefile
                  src/lib/testutils/Makefile
                  src/lib/testutils/dhcp_test_lib.sh
                  src/lib/testutils/testdata/Makefile
-		 src/lib/stats/Makefile
-		 src/lib/stats/tests/Makefile
+                 src/lib/stats/Makefile
+                 src/lib/stats/tests/Makefile
                  src/lib/util/Makefile
                  src/lib/util/io/Makefile
                  src/lib/util/python/Makefile

src/lib/cc/data.h

@@ -677,7 +677,6 @@ public:
 
     /// @brief Returns number of stored elements
     ///
-    /// @note: this is costly operation for a map!
     /// @return number of elements.
     size_t size() const {
         return (m.size());

src/lib/stats/Makefile.am

 SUBDIRS = . tests
 
 AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
+AM_CPPFLAGS += $(BOOST_INCLUDES)
 AM_CXXFLAGS=$(KEA_CXXFLAGS)
 
 lib_LTLIBRARIES = libkea-stats.la
@@ -8,13 +9,9 @@ libkea_stats_la_SOURCES = observation.h observation.cc
 libkea_stats_la_SOURCES += context.h context.cc
 libkea_stats_la_SOURCES += stats_mgr.h stats_mgr.cc
 
+libkea_stats_la_CPPFLAGS = $(AM_CPPFLAGS)
 libkea_stats_la_LIBADD  = $(top_builddir)/src/lib/cc/libkea-cc.la
-
-# This library seem to be required when certain methods are called from
-# boost::posix_time. In particular, to_simple_string(ptime) and
-# to_simple_string(time_duration) require it on Ubuntu. It doesn't seem
-# to be required on Mac OS, though.
-libkea_stats_la_LIBADD += -lboost_date_time
+libkea_stats_la_LIBADD += $(top_builddir)/src/lib/util/libkea-util.la
 
 libkea_stats_includedir = $(includedir)/$(PACKAGE_NAME)/stats
 libkea_stats_include_HEADERS = stats_mgr.h

src/lib/stats/observation.cc

-#include <boost/date_time/posix_time/posix_time.hpp>
+// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
 #include <stats/observation.h>
+#include <util/boost_time_utils.h>
 #include <cc/data.h>
+#include <boost/date_time/posix_time/posix_time.hpp>
+#include <boost/date_time/gregorian/gregorian.hpp>
 #include <utility>
 
 using namespace std;
@@ -10,17 +26,17 @@ using namespace boost::posix_time;
 namespace isc {
 namespace stats {
 
-Observation::Observation(const std::string& name, uint64_t value)
+Observation::Observation(const std::string& name, const uint64_t value)
     :name_(name), type_(STAT_INTEGER) {
     setValue(value);
 }
 
-Observation::Observation(const std::string& name, double value)
+Observation::Observation(const std::string& name, const double value)
     :name_(name), type_(STAT_FLOAT) {
     setValue(value);
 }
 
-Observation::Observation(const std::string& name, StatsDuration value)
+Observation::Observation(const std::string& name, const StatsDuration value)
     :name_(name), type_(STAT_DURATION) {
     setValue(value);
 }
@@ -30,17 +46,17 @@ Observation::Observation(const std::string& name, const std::string& value)
     setValue(value);
 }
 
-void Observation::addValue(uint64_t value) {
+void Observation::addValue(const uint64_t value) {
     IntegerSample current = getInteger();
     setValue(current.first + value);
 }
 
-void Observation::addValue(double value) {
+void Observation::addValue(const double value) {
     FloatSample current = getFloat();
     setValue(current.first + value);
 }
 
-void Observation::addValue(StatsDuration value) {
+void Observation::addValue(const StatsDuration& value) {
     DurationSample current = getDuration();
     setValue(current.first + value);
 }
@@ -50,15 +66,15 @@ void Observation::addValue(const std::string& value) {
     setValue(current.first + value);
 }
 
-void Observation::setValue(uint64_t value) {
+void Observation::setValue(const uint64_t value) {
     setValueInternal(value, integer_samples_, STAT_INTEGER);
 }
 
-void Observation::setValue(double value) {
+void Observation::setValue(const double value) {
     setValueInternal(value, float_samples_, STAT_FLOAT);
 }
 
-void Observation::setValue(StatsDuration value) {
+void Observation::setValue(const StatsDuration& value) {
     setValueInternal(value, duration_samples_, STAT_DURATION);
 }
 
@@ -140,20 +156,6 @@ std::string Observation::typeToText(Type type) {
     return (tmp.str());
 }
 
-std::string
-Observation::ptimeToText(ptime t) {
-
-    // The alternative would be to call to_simple_string(ptime), but unfortunately
-    // that requires linking with boost libraries.
-
-    return (to_simple_string(t));
-}
-
-std::string
-Observation::durationToText(StatsDuration dur) {
-    return (to_simple_string(dur));
-}
-
 isc::data::ConstElementPtr
 Observation::getJSON() const {
 
@@ -168,29 +170,30 @@ Observation::getJSON() const {
     case STAT_INTEGER: {
         IntegerSample s = getInteger();
         value = isc::data::Element::create(static_cast<int64_t>(s.first));
-        timestamp = isc::data::Element::create(ptimeToText(s.second));
+        timestamp = isc::data::Element::create(isc::util::ptimeToText(s.second));
         break;
     }
     case STAT_FLOAT: {
         FloatSample s = getFloat();
         value = isc::data::Element::create(s.first);
-        timestamp = isc::data::Element::create(ptimeToText(s.second));
+        timestamp = isc::data::Element::create(isc::util::ptimeToText(s.second));
         break;
     }
     case STAT_DURATION: {
         DurationSample s = getDuration();
-        value = isc::data::Element::create(durationToText(s.first));
-        timestamp = isc::data::Element::create(ptimeToText(s.second));
+        value = isc::data::Element::create(isc::util::durationToText(s.first));
+        timestamp = isc::data::Element::create(isc::util::ptimeToText(s.second));
         break;
     }
     case STAT_STRING: {
         StringSample s = getString();
         value = isc::data::Element::create(s.first);
-        timestamp = isc::data::Element::create(ptimeToText(s.second));
+        timestamp = isc::data::Element::create(isc::util::ptimeToText(s.second));
         break;
     }
     default:
-        isc_throw(InvalidStatType, "Unknown stat type: " << typeToText(type_));
+        isc_throw(InvalidStatType, "Unknown statistic type: "
+                  << typeToText(type_));
     };
 
     entry->add(value);
@@ -221,7 +224,8 @@ void Observation::reset() {
         return;
     }
     default:
-        isc_throw(InvalidStatType, "Unknown stat type: " << typeToText(type_));
+        isc_throw(InvalidStatType, "Unknown statistic type: "
+                  << typeToText(type_));
     };
 }
 

src/lib/stats/observation.h

@@ -15,12 +15,13 @@
 #ifndef OBSERVATION_H
 #define OBSERVATION_H
 
-#include <boost/shared_ptr.hpp>
-#include <exceptions/exceptions.h>
 #include <cc/data.h>
+#include <exceptions/exceptions.h>
+#include <boost/shared_ptr.hpp>
 #include <boost/date_time/time_duration.hpp>
 #include <boost/date_time/posix_time/posix_time_types.hpp>
 #include <list>
+#include <stdint.h>
 
 namespace isc {
 namespace stats {
@@ -37,11 +38,7 @@ public:
 
 /// @brief Defines duration resolution
 ///
-/// Boost offers a base boost::posix_time::time_duration class, that has specific
-/// implementations: boost::posix_time::{hours,minutes,seconds,millisec,microsec,
-/// nanosec}. For statistics purposes, the most appropriate choice seems to be
-/// microseconds precision, so we'll stick with that.
-typedef boost::posix_time::microsec::time_duration StatsDuration;
+typedef boost::posix_time::time_duration StatsDuration;
 
 /// @defgroup stat_samples Specifies supported observation types.
 ///
@@ -101,19 +98,19 @@ class Observation {
     ///
     /// @param name observation name
     /// @param value integer value observed.
-    Observation(const std::string& name, uint64_t value);
+    Observation(const std::string& name, const uint64_t value);
 
     /// @brief Constructor for floating point observations
     ///
     /// @param name observation name
     /// @param value floating point value observed.
-    Observation(const std::string& name, double value);
+    Observation(const std::string& name, const double value);
 
     /// @brief Constructor for duration observations
     ///
     /// @param name observation name
     /// @param value duration observed.
-    Observation(const std::string& name, StatsDuration value);
+    Observation(const std::string& name, const StatsDuration value);
 
     /// @brief Constructor for string observations
     ///
@@ -125,43 +122,43 @@ class Observation {
     ///
     /// @param value integer value observed
     /// @throw InvalidStatType if statistic is not integer
-    void setValue(uint64_t value);
+    void setValue(const uint64_t value);
 
     /// @brief Records absolute floating point observation
     ///
     /// @param value floating point value observed
     /// @throw InvalidStatType if statistic is not fp
-    void setValue(double value);
+    void setValue(const double value);
 
     /// @brief Records absolute duration observation
     ///
     /// @param value duration value observed
     /// @throw InvalidStatType if statistic is not time duration
-    void setValue(StatsDuration duration);
+    void setValue(const StatsDuration& duration);
 
     /// @brief Records absolute string observation
     ///
     /// @param value string value observed
     /// @throw InvalidStatType if statistic is not a string
-    void setValue(const std::string& value = "");
+    void setValue(const std::string& value);
 
     /// @brief Records incremental integer observation
     ///
     /// @param value integer value observed
     /// @throw InvalidStatType if statistic is not integer
-    void addValue(uint64_t value = 1);
+    void addValue(const uint64_t value = 1);
 
     /// @brief Records incremental floating point observation
     ///
     /// @param value floating point value observed
     /// @throw InvalidStatType if statistic is not fp
-    void addValue(double value = 1.0f);
+    void addValue(const double value = 1.0f);
 
     /// @brief Records incremental duration observation
     ///
     /// @param value duration value observed
     /// @throw InvalidStatType if statistic is not time duration
-    void addValue(StatsDuration value = StatsDuration(0,0,0,0));
+    void addValue(const StatsDuration& value = StatsDuration(0,0,1,0));
 
     /// @brief Records incremental string observation.
     ///
@@ -208,20 +205,12 @@ class Observation {
     /// @return textual name of statistic type
     static std::string typeToText(Type type);
 
-    /// @brief Converts ptime structure to text
-    /// @return a string representing time
-    static std::string ptimeToText(boost::posix_time::ptime time);
-
-    /// @brief Converts StatsDuration to text
-    /// @return a string representing time
-    static std::string durationToText(StatsDuration dur);
-
     /// @brief Returns observation name
     std::string getName() const {
         return (name_);
     }
 
- protected:
+private:
     /// @brief Records absolute sample (internal version)
     ///
     /// This method records an absolute value of an observation.
@@ -238,7 +227,7 @@ class Observation {
     void setValueInternal(SampleType value, StorageType& storage,
                           Type exp_type);
 
-    /// @brief Returns a sample
+    /// @brief Returns a sample (internal version)
     ///
     /// @tparam SampleType type of sample (e.g. IntegerSample)
     /// @tparam StorageType type of storage (e.g. list<IntegerSample>)

src/lib/stats/stats_mgr.cc

@@ -30,30 +30,30 @@ StatsMgr::StatsMgr()
 
 }
 
-void StatsMgr::setValue(const std::string& name, uint64_t value) {
+void StatsMgr::setValue(const std::string& name, const uint64_t value) {
     setValueInternal(name, value);
 }
 
-void StatsMgr::setValue(const std::string& name, double value) {
+void StatsMgr::setValue(const std::string& name, const double value) {
     setValueInternal(name, value);
 }
 
-void StatsMgr::setValue(const std::string& name, StatsDuration value) {
+void StatsMgr::setValue(const std::string& name, const StatsDuration& value) {
     setValueInternal(name, value);
 }
 void StatsMgr::setValue(const std::string& name, const std::string& value) {
     setValueInternal(name, value);
 }
 
-void StatsMgr::addValue(const std::string& name, uint64_t value) {
+void StatsMgr::addValue(const std::string& name, const uint64_t value) {
     addValueInternal(name, value);
 }
 
-void StatsMgr::addValue(const std::string& name, double value) {
+void StatsMgr::addValue(const std::string& name, const double value) {
     addValueInternal(name, value);
 }
 
-void StatsMgr::addValue(const std::string& name, StatsDuration value) {
+void StatsMgr::addValue(const std::string& name, const StatsDuration& value) {
     addValueInternal(name, value);
 }
 
@@ -63,24 +63,24 @@ void StatsMgr::addValue(const std::string& name, const std::string& value) {
 
 ObservationPtr StatsMgr::getObservation(const std::string& name) const {
     /// @todo: Implement contexts.
-    // Currently we keep everyting is a global context.
+    // Currently we keep everyting in a global context.
     return (global_->get(name));
 }
 
 void StatsMgr::addObservation(const ObservationPtr& stat) {
     /// @todo: Implement contexts.
-    // Currently we keep everyting is a global context.
+    // Currently we keep everyting in a global context.
     return (global_->add(stat));
 }
 
 bool StatsMgr::deleteObservation(const std::string& name) {
     /// @todo: Implement contexts.
-    // Currently we keep everyting is a global context.
+    // Currently we keep everyting in a global context.
     return (global_->del(name));
 }
 
 void StatsMgr::setMaxSampleAge(const std::string& ,
-                               boost::posix_time::time_duration) {
+                               const StatsDuration&) {
     isc_throw(NotImplemented, "setMaxSampleAge not implemented");
 }
 

src/lib/stats/stats_mgr.h

@@ -60,54 +60,54 @@ class StatsMgr : public boost::noncopyable {
     ///
     /// @{
 
-    /// @brief Records absolute integer observation
+    /// @brief Records absolute integer observation.
     ///
     /// @param name name of the observation
     /// @param value integer value observed
     /// @throw InvalidStatType if statistic is not integer
-    void setValue(const std::string& name, uint64_t value);
+    void setValue(const std::string& name, const uint64_t value);
 
-    /// @brief Records absolute floating point observation
+    /// @brief Records absolute floating point observation.
     ///
     /// @param name name of the observation
     /// @param value floating point value observed
     /// @throw InvalidStatType if statistic is not fp
-    void setValue(const std::string& name, double value);
+    void setValue(const std::string& name, const double value);
 
-    /// @brief Records absolute duration observation
+    /// @brief Records absolute duration observation.
     ///
     /// @param name name of the observation
     /// @param value duration value observed
     /// @throw InvalidStatType if statistic is not time duration
-    void setValue(const std::string& name, StatsDuration value);
+    void setValue(const std::string& name, const StatsDuration& value);
 
-    /// @brief Records absolute string observation
+    /// @brief Records absolute string observation.
     ///
     /// @param name name of the observation
     /// @param value string value observed
     /// @throw InvalidStatType if statistic is not a string
     void setValue(const std::string& name, const std::string& value);
 
-    /// @brief Records incremental integer observation
+    /// @brief Records incremental integer observation.
     ///
     /// @param name name of the observation
     /// @param value integer value observed
     /// @throw InvalidStatType if statistic is not integer
-    void addValue(const std::string& name, uint64_t value);
+    void addValue(const std::string& name, const uint64_t value);
 
-    /// @brief Records incremental floating point observation
+    /// @brief Records incremental floating point observation.
     ///
     /// @param name name of the observation
     /// @param value floating point value observed
     /// @throw InvalidStatType if statistic is not fp
-    void addValue(const std::string& name, double value);
+    void addValue(const std::string& name, const double value);
 
-    /// @brief Records incremental duration observation
+    /// @brief Records incremental duration observation.
     ///
     /// @param name name of the observation
     /// @param value duration value observed
     /// @throw InvalidStatType if statistic is not time duration
-    void addValue(const std::string& name, StatsDuration time);
+    void addValue(const std::string& name, const StatsDuration& time);
 
     /// @brief Records incremental string observation.
     ///
@@ -116,8 +116,7 @@ class StatsMgr : public boost::noncopyable {
     /// @throw InvalidStatType if statistic is not a string
     void addValue(const std::string& name, const std::string& value);
 
-    /// @brief determines whether a given statistic is kept as a single value
-    ///        or as a number of values
+    /// @brief Determines maximum age of samples.
     ///
     /// Specifies that statistic name should be stored not as a single value,
     /// but rather as a set of values. duration determines the timespan.
@@ -131,10 +130,9 @@ class StatsMgr : public boost::noncopyable {
     /// call setMaxSampleAge("incoming-packets", time_duration(0,5,0,0));
     /// to revert statistic to a single value, call:
     /// setMaxSampleAge("incoming-packets" time_duration(0,0,0,0))
-    void setMaxSampleAge(const std::string& name,
-                         boost::posix_time::time_duration duration);
+    void setMaxSampleAge(const std::string& name, const StatsDuration& duration);
 
-    /// @brief determines how many samples of a given statistic should be kept.
+    /// @brief Determines how many samples of a given statistic should be kept.
     ///
     /// Specifies that statistic name should be stored not as single value, but
     /// rather as a set of values. In this form, at most max_samples will be kept.
@@ -164,6 +162,7 @@ class StatsMgr : public boost::noncopyable {
     bool reset(const std::string& name);
 
     /// @brief Removes specified statistic.
+    ///
     /// @param name name of the statistic to be removed.
     /// @return true if successful, false if there's no such statistic
     bool del(const std::string& name);
@@ -175,20 +174,23 @@ class StatsMgr : public boost::noncopyable {
     void removeAll();
 
     /// @brief Returns number of available statistics.
+    ///
     /// @return number of recorded statistics.
     size_t count() const;
 
-    /// @brief Returns a single statistic as a JSON structure
+    /// @brief Returns a single statistic as a JSON structure.
+    ///
     /// @return JSON structures representing a single statistic
     isc::data::ConstElementPtr get(const std::string& name) const;
 
-    /// @brief Returns all statistics as a JSON structure
+    /// @brief Returns all statistics as a JSON structure.
+    ///
     /// @return JSON structures representing all statistics
     isc::data::ConstElementPtr getAll() const;
 
     /// @}
 
-    /// @brief Returns an observation
+    /// @brief Returns an observation.
     ///
     /// Used in testing only. Production code should use @ref get() method.
     /// @param name name of the statistic
@@ -197,13 +199,13 @@ class StatsMgr : public boost::noncopyable {
 
  private:
 
-    /// @brief Sets a given statistic to specified value (internal version)
+    /// @brief Sets a given statistic to specified value (internal version).
     ///
     /// This template method sets statistic identified by name to a value
     /// specified by value. This internal method is used by public @ref setValue
     /// methods.
     ///
-    /// @tparam DataType one of uint64_t, double DurationStat or string
+    /// @tparam DataType one of uint64_t, double, StatsDuration or string
     /// @param name name of the statistic
     /// @param value specified statistic will be set to this value
     /// @throw InvalidStatType is statistic exists and has a different type.
@@ -218,13 +220,13 @@ class StatsMgr : public boost::noncopyable {
         }
     }
 
-    /// @brief Adds specified value to a given statistic (internal version)
+    /// @brief Adds specified value to a given statistic (internal version).
     ///
     /// This template method adds specified value to a given statistic (identified
     /// by name to a value). This internal method is used by public @ref setValue
     /// methods.
     ///
-    /// @tparam DataType one of uint64_t, double DurationStat or string
+    /// @tparam DataType one of uint64_t, double, StatsDuration or string
     /// @param name name of the statistic
     /// @param value specified statistic will be set to this value
     /// @throw InvalidStatType is statistic exists and has a different type.
@@ -244,25 +246,25 @@ class StatsMgr : public boost::noncopyable {
         }
     }
 
-    /// @brief Private constructor
+    /// @brief Private constructor.
     /// StatsMgr is a singleton. It should be accessed using @ref instance
     /// method.
     StatsMgr();
 
-    /// @brief Adds a new observation
+    /// @brief Adds a new observation.
     ///
     /// That's an utility method used by public @ref setValue() and
     /// @ref addValue() methods.
     /// @param obs observation
     void addObservation(const ObservationPtr& o);
 
-    /// @brief Tries to delete an observation
+    /// @brief Tries to delete an observation.
     ///
     /// @param name of the statistic to be deleted
     /// @return true if deleted, false if not found
     bool deleteObservation(const std::string& name);
 
-    // This is a global context. All stats will initially be stored here.
+    // This is a global context. All statistics will initially be stored here.
     StatContextPtr global_;
 };
 

src/lib/stats/tests/Makefile.am

@@ -25,8 +25,8 @@ libstats_unittests_CXXFLAGS = $(AM_CXXFLAGS)
 libstats_unittests_LDADD  = $(top_builddir)/src/lib/stats/libkea-stats.la
 libstats_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
 libstats_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
+libstats_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
 libstats_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
-libstats_unittests_LDADD  += -lboost_date_time
 libstats_unittests_LDADD += $(GTEST_LDADD)
 
 endif

src/lib/stats/tests/observation_unittest.cc

@@ -16,7 +16,7 @@
 
 #include <stats/observation.h>
 #include <exceptions/exceptions.h>
-
+#include <util/boost_time_utils.h>
 #include <boost/shared_ptr.hpp>
 #include <boost/date_time/posix_time/posix_time_types.hpp>
 #include <gtest/gtest.h>
@@ -32,8 +32,15 @@ using namespace boost::posix_time;
 
 namespace {
 
+/// @brief Test class for Observation
+///
+/// This simple fixture class initializes four observations:
+/// a (integer), b (float), c(time duration) and d (string).
 class ObservationTest : public ::testing::Test {
 public:
+
+    /// @brief Constructor
+    /// Initializes four observations.
     ObservationTest()
         :a("alpha", static_cast<uint64_t>(1234)), // integer
          b("beta", 12.34), // float
@@ -47,8 +54,8 @@ public:
     Observation d;
 };
 
-// Basic tests for V4 functionality. This test checks whether parameters
-// passed to constructor initialize the object properly.
+// Basic tests for the Obseration constructors. This test checks whether
+// parameters passed to the constructor initialize the object properly.
 TEST_F(ObservationTest, constructor) {
 
     EXPECT_EQ(Observation::STAT_INTEGER, a.getType());
@@ -137,12 +144,12 @@ TEST_F(ObservationTest, addValue) {
 
 // Test checks whether timing is reported properly.