Browse Source

[3798] Changes after review:

 - comments in StatsMgr improved
 - added unit-test for CfgMgr::clear() and stats
 - copyright years updated
Tomek Mrugalski 10 years ago
parent
commit
70f37c3b4f

+ 1 - 1
src/lib/dhcpsrv/cfg_subnets4.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-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

+ 3 - 0
src/lib/dhcpsrv/cfgmgr.cc

@@ -113,6 +113,9 @@ CfgMgr::ensureCurrentAllocated() {
 
 void
 CfgMgr::clear() {
+    if (configuration_) {
+        configuration_->removeStatistics();
+    }
     configs_.clear();
     ensureCurrentAllocated();
 }

+ 1 - 1
src/lib/dhcpsrv/cfgmgr.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-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

+ 27 - 1
src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-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
@@ -615,6 +615,32 @@ TEST_F(CfgMgrTest, commitStats) {
     EXPECT_EQ(128, total_addrs->getInteger().first);
 }
 
+// This test verifies that once the configuration is cleared, the statistics
+// are removed.
+TEST_F(CfgMgrTest, clearStats) {
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+    StatsMgr& stats_mgr = StatsMgr::instance();
+
+    // Let's prepare the "old" configuration: a subnet with id 123
+    // and pretent there ware addresses assigned, so statistics are non-zero.
+    Subnet4Ptr subnet1(new Subnet4(IOAddress("192.1.2.0"), 24, 1, 2, 3, 123));
+    CfgSubnets4Ptr subnets = cfg_mgr.getStagingCfg()->getCfgSubnets4();
+    subnets->add(subnet1);
+    cfg_mgr.commit();
+    stats_mgr.addValue("subnet[123].total-addresses", static_cast<int64_t>(256));
+    stats_mgr.setValue("subnet[123].assigned-addresses", static_cast<int64_t>(150));
+
+    // The stats should be there.
+    EXPECT_TRUE(stats_mgr.getObservation("subnet[123].total-addresses"));
+    EXPECT_TRUE(stats_mgr.getObservation("subnet[123].assigned-addresses"));
+
+    // Let's remove all configurations
+    cfg_mgr.clear();
+
+    // The stats should not be there anymore.
+    EXPECT_FALSE(stats_mgr.getObservation("subnet[123].total-addresses"));
+    EXPECT_FALSE(stats_mgr.getObservation("subnet[123].assigned-addresses"));
+}
 
 /// @todo Add unit-tests for testing:
 /// - addActiveIface() with invalid interface name

+ 27 - 3
src/lib/stats/stats_mgr.h

@@ -49,6 +49,23 @@ namespace stats {
 /// is today, except happening in a separate thread. One unsolved issue in
 /// this approach is how to extract data, but that will remain unsolvable
 /// until we get the control socket implementation.
+///
+/// Statistics Manager does not use logging by design. The reasons are:
+/// - performance impact (logging every observation would degrade performance
+///   significantly. While it's possible to log on sufficiently high debug
+///   level, such a log would be not that useful)
+/// - dependency (statistics are intended to be a lightweight library, adding
+///   dependency on libkea-log, which has its own dependecies, including
+///   external log4cplus, is against 'lightweight' design)
+/// - if logging of specific statistics is warranted, it is recommended to
+///   add log entries in the code that calls StatsMgr.
+/// - enabling logging in StatsMgr does not offer fine tuning. It would be
+///   either all or nothing. Adding logging entries only when necessary
+///   in the code that uses StatsMgr gives better granularity.
+///
+/// If this decision is revisited in the futere, the most universal places
+/// for adding logging have been marked in @ref addValueInternal and
+/// @ref setValueInternal.
 class StatsMgr : public boost::noncopyable {
  public:
 
@@ -200,9 +217,12 @@ class StatsMgr : public boost::noncopyable {
 
     /// @brief Generates statistic name in a given context
     ///
-    /// example:
-    /// generateName("subnet", 123, "received-packets") will return
-    /// subnet[123].received-packets.
+    /// Example:
+    /// @code
+    /// generateName("subnet", 123, "received-packets");
+    /// @endcode
+    /// will return subnet[123].received-packets. Any printable type
+    /// can be used as index.
     ///
     /// @tparam Type any type that can be used to index contexts
     /// @param context name of the context (e.g. 'subnet')
@@ -231,6 +251,8 @@ class StatsMgr : public boost::noncopyable {
     /// @throw InvalidStatType is statistic exists and has a different type.
     template<typename DataType>
     void setValueInternal(const std::string& name, DataType value) {
+
+        // If we want to log each observation, here would be the best place for it.
         ObservationPtr stat = getObservation(name);
         if (stat) {
             stat->setValue(value);
@@ -252,6 +274,8 @@ class StatsMgr : public boost::noncopyable {
     /// @throw InvalidStatType is statistic exists and has a different type.
     template<typename DataType>
     void addValueInternal(const std::string& name, DataType value) {
+
+        // If we want to log each observation, here would be the best place for it.
         ObservationPtr existing = getObservation(name);
         if (!existing) {
             // We tried to add to a non-existing statistic. We can recover from