BIND 10 #2155: Collect query/response statistics items in Auth module

[BIND 10 Development]

#2155: Collect query/response statistics items in Auth module
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  y-aharen
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20121009
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  b10-auth                           |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => y-aharen


Comment:

 Hello

 I'm glad the biggest issue was sorted out. However, some more issues still
 remain or new ones appear.

 The headers in EXTRA_DIST look unusual. I know it may be reasonable
 solution, since there's no compiled code and no library, but maybe a
 comment to the makefile would be good.

 The note about SIG(0) still applies:
 {{{
         // statistics: check TSIG attributes
         // SIG(0) is currently not implemented in Auth
 }}}

 It seems the SIG(0) is some kind of signing algorithm or something? But if
 so, it is unclear how it relates to the surrounding code and why it is
 important for the statistics code that it is not implemented in Auth.

 Also, what is the reason for this bit of code having a separate block
 around it?
 {{{#!c++
         //     note: This can only be reliable after TSIG check succeeds.
         {
             ConstEDNSPtr edns = message.getEDNS();
             if (edns != NULL) {
                 stats_attrs.setQueryEDNS(true, edns->getVersion() == 0);
                 stats_attrs.setQueryDO(edns->getDNSSECAwareness());
             }
         }
 }}}

 In this bit of code, how is it possible for the qptr to be NULL? If it is
 NULL, what does that mean? And if it can't, should we assert for it
 instead?
 {{{#!c++
     const QuestionIterator qiter = response.beginQuestion();
     if (qiter != response.endQuestion()) {
         // get the first and only question section
         const QuestionPtr qptr = *qiter;
         if (qptr != NULL) {
 }}}

 In this code, should we distinguish NXDOMAIN as well as NXRRSET?
 {{{#!c++
 if (rcode == Rcode::NOERROR_CODE) {
     if (answer_rrs > 0) {
         // QrySuccess
         server_qr_counter_.inc(QR_QRYSUCCESS);
     } else {
         if (is_aa_set) {
             // QryNxrrset
             server_qr_counter_.inc(QR_QRYNXRRSET);
         } else {
             // QryReferral
             server_qr_counter_.inc(QR_QRYREFERRAL);
         }
     }
 } else if (rcode == Rcode::REFUSED_CODE) {
     // AuthRej
     server_qr_counter_.inc(QR_QRYREJECT);
 }
 }}}

 There are magic constants in the code. Maybe they should be defined as
 constants somewhere. Also, the comments like „qtype 0..257“ are really
 useless. They say the obvious thing about the code, exactly what is
 written there, but don't clarify anything about the reason for the code or
 such.:
 {{{#!c++
 if (qtype < 258) {
     // qtype 0..257
     qtype_type = QRQTypeToQRCounterType[qtype];
 } else if (qtype < 32768) {
     // qtype 258..32767
     qtype_type = QR_QTYPE_OTHER;
 } else if (qtype < 32770) {
     // qtype 32768..32769
     qtype_type = QR_QTYPE_TA + (qtype - 32768);
 } else {
     // qtype 32770..65535
     qtype_type = QR_QTYPE_OTHER;
 }
 }}}

 {{{#!c++
 if (rcode < 23) {
     // rcode 0..22
 }}}

 I smell something bad if I see code like this. This is very long and easy
 to make the number of lines wrong in one way or another. And, when you do
 it wrong, nothing breaks, you just get wrong answers:
 {{{#!c++
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
     QR_QTYPE_OTHER,
 }}}

 It would be better to build the array from bunch of position-value pairs
 (omitting the OTHER ones, that'd be there in case no pair would have that
 position). Or even better, just from the list of „special“ types (like A
 and SOA and so on) and pull the positions from the dns++ library.

 The test functions `checkAllRcodeCountersZeroExcept` and
 `checkAllRcodeCountersZero` look very similar. Would it be possible to
 share the code between them somehow? Maybe implement the
 `checkAllRcodeCountersZero` as a call to `checkAllRcodeCountersZero` with
 some random RCode and 0?

 I don't understand the purpose of this function. What does it do? Maybe
 comment it?
 {{{#!c++
 void
 expectCounterItem(ConstElementPtr stats,
                   const std::string& item, const int expected) {
     ConstElementPtr value(Element::create(0));
     if (item == "queries.udp" || item == "queries.tcp" || expected != 0) {
         ASSERT_TRUE(stats->find(item, value)) << "    Item: " << item;
         value = stats->find(item);
         EXPECT_EQ(expected, value->intValue()) << "    Item: " << item;
     } else {
         ASSERT_FALSE(stats->find(item, value)) << "    Item: " << item <<
             std::endl << "   Value: " << value->intValue();
     }
 }
 }}}

 This comment seems wrong, `request.tcp` is both in incremented and not
 incremented. Should the first line say `request.udp`?
 {{{#!c++
 // After processing the UDP query, these counters should be incremented:
 //   request.tcp, opcode.query, qtype.ns, rcode.refused, response
 // and these counters should not be incremented:
 //   request.tcp
 }}}

 Also, the same test as above and most of the similar check one value less
 than what is being listed in the comments. Is it forgotten, or is it some
 trick I don't see?

 May I suggest removing useless code, like empty constructors and
 destructors?
 {{{#!c++
     CountersTest() : counters() {}
     ~CountersTest() {}
 }}}

 {{{#!c++
 Counters::~Counters() {}
 }}}

 {{{#!c++
 inline QRAttributes::~QRAttributes()
 {}
 }}}

 {{{#!c++
 inline Counter::~Counter() {}
 }}}

 {{{#!c++
 inline ~ConstIterator() {}
 }}}

 {{{#!c++
 inline CounterDictionary::~CounterDictionary() {}
 }}}

 Also, the canonical way to inline methods is not the `inline` keyword, but
 having them inside the class definition itself, like this:
 {{{#!c++
 class Class {
 public:
         void thisMethodGetsInlined() {
                 ...
         }
 };
 }}}

 Does this function return false every time? Is there any use of the return
 value then?
 {{{#!c++
 bool
 checkCountersAllZeroExcept(const isc::data::ConstElementPtr counters,
                            const std::set<std::string>& except_for) {
 }}}

 The constructor and `reset` method of `QRAttributes` look very similar.
 Would it be better to just call reset from the constructor?

 I think these are auto-generated by compiler when needed:
 {{{#!c++
 inline ConstIterator& operator=(const ConstIterator& source) {
     iterator_ = source.iterator_;
     return (*this);
 }

 inline ConstIterator(const ConstIterator& source) :
     iterator_(source.iterator_)
 {}
 }}}

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