Opened 8 years ago

Closed 7 years ago

#1958 closed enhancement (fixed)

Implement perfdhcp statistics component

Reported by: stephen Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20120730
Component: perfdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 16 Add Hours to Ticket: 2
Total Hours: 67 Internal?: no

Description

Centralise the various counters maintained by perfdhcp into a separate class and implement associated functionality (such as printing summary statistics).

Subtickets

Change History (12)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20120703

comment:2 Changed 7 years ago by marcin

  • Estimated Difficulty changed from 0 to 16
  • Owner set to marcin
  • Status changed from new to accepted

comment:3 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 0 to 48
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 48

Stats Manager has been implemented. Please review.

comment:4 Changed 7 years ago by marcin

  • Owner changed from UnAssigned to marcin
  • Status changed from reviewing to accepted

Please hold off review. Implementation lacks statistics printing functionality. Implementation is in progress.

comment:5 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 48 to 8
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 48 to 56

A set of statistics printing functions have been added.
This imposed a couple of changes to Statistics Manager itself. Major changes are:

  • Moving sent and matched packets to archived list for diagnostics
  • Adding another search index to packet list (use transaction id)
  • Return average and std dev values rather than sums of delays and sum of squre delays
  • Change uint64_t return types to unsiged long long to get rid of casts when using printf
  • dhcp::Pkt6::getTransid is const so as it can be used by new index and for consistency with dhcp::Pkt4::getTransid.

Note: it seems that other getter functions in dhcp::Pkt6 are not const. It will be actually good to change other getters to const too but I did not include this change as a part of this ticket.

Please review branch trac1958.

comment:6 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 48755af6bfc85aa10e5eea2da1bde9478914e8df

tests/tools/perfdhcp/stats_mgr.h
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
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.

comment:8 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 8 to 9
  • Owner changed from marcin to stephen
  • Total Hours changed from 56 to 65

Q: PktList? is perhaps better named PktCollection? (as the container is not a list).
A: The multi_index_container can be perceived as a list and elements are accessed in the same way as we access std::list. It also allows to insert and erase elements from the middle of it. What's added is the second (non-sequencial) index that allows speedup search of the element by its transaction id. Otherwise it acts the same as list so I would keep it as it is.

Q: passDOPacketsWithDelay(): do we need similar functions for the REQUEST-ACK, SOLICIT-ADVERTISE, REQUEST-RESPONSE exchanges?
A: I don't think we do. From the stats manager's perspective the different exchange is just a different enum. In this case, having DISCOVER-OFFER is sufficient to test its functionality. There is no value added to generalise this function at this point. Should this need ever arise, we can change it.

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 2175b280a686cc4aec2cf72c21f7ce839bf609ee

tests/tools/perfdhcp/stats_mgr.h
getName()/getValue() etc - return value should be in brackets (as is the case in methods such as getMinDelay()).

ExchangeStats constructor requires description of archive_enabled parameter.

I'm a bit unclear about the rcvd_packets_num_ counter. It had previously been incremented in appendRcvd(), but is now incremented in findSent(). Presumably this means that the counter is the number of packets received that corresponded to packets sent, and not the number of packets received.

StatsMgr constructor. Only one version is needed - set the default value for the archive_enabled parameter to false.

comment:10 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 9 to 2
  • Owner changed from marcin to stephen
  • Total Hours changed from 65 to 67

I corrected the code as suggested.

Regarding the rcvd_packets_num_ counter....
The fact that increment operation was moved from appendRcvd() to findSent() (which has been now renamed to matchPackets()) does not change the behavior. This is because appendSent() was called only in case the received packet was matched with sent packet. I had to move this out from appendRcvd() because when so called 'archive mode' is disabled I neither store sent nor received packets once they are matched. In other words I never call appendRecvd() when archive mode is disabled but I still want to increment the counter. It seems to be the most natural place to do it where it is done now.

I also decided to rename the findSent() function to matchPackets() which in my opinion better reflects the purpose of it. Not only does it find the 'sent' packet but also it does other things like increment counters.

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 9759be762e000ccdf9e17ca890fdb990bd773c02

Looks OK, please merge.

comment:12 Changed 7 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Code merged to master.

Note: See TracTickets for help on using tickets.