BIND 10 #2154: Create a set of counters to hold auth statistics items

[BIND 10 Development]

#2154: Create a set of counters to hold auth statistics items
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120904
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  b10-auth                           |  Estimated Difficulty:  8
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 In an attempt of reviewing #2157, a number of questions and concerns
 occurred to me, and I realized they are mainly related to this ticket.
 So I'm making my own comments here.

 I have some concerns with the current approach implemented in this
 branch, mainly related to scalability and (per query/request) update
 performance.

 Regarding scalability, I also don't think it makes much sense to
 define enum for each known RR type.  Also, the large if block in
 `CountersImpl::inc` makes the code unreadable and possibly less
 efficient (large functions generally make it slower.  Many 'if-else's
 are also not performance friendly).

 I'm also not sure if it makes sense to collect attributes first and
 then update the corresponding statistics counters.  This two-stage
 operation has its own overhead, and we need to have yet another set of
 if-else's in CountersImpl::inc().

 Essentially, statistics counters are a simple mapping from "some ID"
 to an integer, so things like
 {{{#!cpp
                 stats_attrs.setQueryDO(edns->getDNSSECAwareness());
                 // and eventually do this in CountersImpl::inc().
     if (qrattrs.req_is_dnssec_ok_) {
         server_qr_counter_.inc(QR_REQUEST_DNSSEC_OK);
     }
 }}}
 could be a simple increment of an element in an array or vector:
 {{{#!cpp
     if (edns->getDNSSECAwareness()) {
         ++server_counters_[QR_REQUEST_DNSSEC_OK];
     }
 }}}
 It would still probably be a good idea to have some level of
 abstraction for safety and (possibly) better extendability, but I'd
 like to minimize the overhead of incrementing a statistics counter as
 small as this simplest one.

 So, if I were to implement this stuff, I'd probably do something like
 this:
 - for generic counters, we use a simple array-type abstraction to
   represent a set of counters.  define enum to indicate specific index
   of the array for a specific statistics counter.  when incrementing
   it, almost directly refer to the array and just do ++
 - for some other DNS specific counters, i.e., per RR type, rcode,
   opcode, define separate simple counter class.  It's also basically a
   simple indexed array, but each class knows how to identify the array
   index for a specific (RRType/Rcode/Opcode/etc) object.  In many
   cases it should be a trivial conversion.  For `RRType`, depending on
   how specific we want to be, we might even make the array
   "extendable" and extend it as we see any uncommon type (but even if
   it makes sense that's beyond the scope of the initial work).  Each
   class also knows how to convert the counters to a JSON format so
   that it can be used to send via a CC channel.

 And, one other minor point I happen to notice.  If we do it:
 {{{#!cpp
         // statistics: check EDNS
         //     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());
             }
         }
 }}}
 I'd pass this `edns` to processNormalQuery (in any case, the outer
 most braces shouldn't be necessary).

 Finally, I'd suggest you check the performance impact on various
 possible approaches using e.g. query_bench.  Maybe my concerns above
 might be marginal in the total overhead of query processing.

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