BIND 10 #1958: Implement perfdhcp statistics component

[BIND 10 Development]

#1958: Implement perfdhcp statistics component
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120730
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  16
                   Keywords:         |           Total Hours:  56
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  8      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 48755af6bfc85aa10e5eea2da1bde9478914e8df

 '''tests/tools/perfdhcp/stats_mgr.h'''[[BR]]
 General: there are a number of methods that take an argument of the form:
 {{{
 const boost::shared_ptr<T> arg
 }}}
 Suggest that the shared_ptr be passed by reference, to avoid the overhead
 of copying of the argument and the associated destruction of the copied
 object, each of which will update the reference counter for the object.

 General: Can we use uint64_t instead of "long long"?

 General: arguments to "return" should be in parentheses.

 General: should be consistent in the layout of the "get" methods: in some
 cases the "return" is on the same line as the method definition, in others
 it is on a separate line. (e.g. getOrderedLookups() v
 getSentPacketsNum()).

 !PktList is perhaps better named !PktCollection (as the container is not a
 list).

 A bit more commenting in the definition of !PktList would help: I see that
 it is indexed by order of insertion and by the hash of the transaction ID:
 but it also seems to be indexed by the raw transaction ID (in which case,
 why is the hash function needed?).  boost::multi_index is not commonly
 used (at least in the BIND 10 project), so we should assume that readers
 of the code will not be familiar with it.

 square_sum_delay_: the name implies the square of the sum, whereas it
 actually represents the sum of the squares.  For this reason, I suggest
 that something like sum_delay_squared_ is a better name.

 appendSent(): a comment describing use of "template" in the statement and
 use of "get<0>()" should be added.  It's the first use of multi-index in
 the file and readers are unlikely to be familiar with its usage.

 findSent(): description of the use of the cache could be better explained.
 In particular, I think that the name "sent_packets_cache_" for a pointer
 to the least-recently sent packet is not good - it implies that it
 represents a collection of packets, not a pointer into them.  Perhaps
 "least_recently_sent_" instead?

 findSent(): not clear why the hash is used if we have also indexed the
 collection of packets with the transaction ID.

 printTimestamps(): all C++ up to the point of "printf".  What happened to
 "cout"? :-)

 The list of archived packets might grow very large if the program is run
 for an extended period.  If we want to keep the functionality, perhaps it
 should be made an option?

 getCounter(): the argument is the key - is this the long or short name
 used to create the counter?

 printRTTStats(): would also include the number of exchanges in the
 statistics to allow a level of confidence to be assigned to the figures.

 '''tests/tools/perfdhcp/tests/stats_mgr_unittest.cc'''[[BR]]
 passDOPacketsWithDelay(): do we need similar functions for the REQUEST-
 ACK, SOLICIT-ADVERTISE, REQUEST-RESPONSE exchanges? (If so, it should be
 possible to generalise this function by taking the packet type as
 argument.)

 All tests: Use the assertion EXPECT_DOUBLE_EQ to compare two double
 precision numbers.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1958#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development