BIND 10 #2157: Create an interface to pass statistics counters in Auth module

[BIND 10 Development]

#2157: Create an interface to pass statistics counters in Auth module
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20121204
                   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):

 I'm still working on it, but giving intermediate feedback.

 '''statistics.h,cc'''

 > > - `QRAttributes::setQueryOpCode` why does it take a (signed) integer?
 > >   what if it's a bogus value like -1 or 16?  isn't it better to take
 > >   an `Opcode` object itself?  In any case the same question applies
 > >   to req_opcode_: why it's an int?
 > Passing isc::dns::OpCode introduces more dependencies in stats code,
 > so I would avoid it.

 But it already depends on `dns::Message`.  Since it implicitly means
 depending on `dns::Opcode`, the explicit reference doesn't really
 introduce any dependency.  Besides, in general, it actually seems to
 be pretty natural that statistics counters for b10-auth use some
 concepts of libdns++.  And,

 > However, it should be at least unsigned. Should we
 > introduce an assertion check?

 if we pass `Opcode` we don't need to worry about invalid values.  It's
 also more type safe; the current interface is susceptible to bugs like
 this:

 {{{#!cpp
     attr.setRequestOpCode(rcode.getCode()); // actually meant opcode
 }}}

 > > - `QRAttributes::setQuerySig()` this interface seems to be a bit ad
 > >   hoc.  how would it be used if and when we support SIG(0)?
 > Sorry if I misunderstood your question, is_sig0 will be used for SIG(0).

 (One of) my points is that it would look awkward to pass "tsig_error"
 if it's not TSIG signed (that could happen if/when we support SIG(0)).
 I'd separate interfaces for the TSIG specific case and for the SIG(0)
 specific case.

 > > - setQueryIPVersion/setQueryTransportProtocol: I'd like to see
 > >   expected values for these in the documentation.  And if unexpected
 > >   values are given?
 > I added notes that describes expected values. If unexpected value is
 given,
 > the counter will not be incremented.

 So we cannot detect a bug that passes an invalid value unless we
 explicitly write tests for such cases.  That doesn't seem to be a good
 interface.

 > > - I'd explicitly include statistics/counter.h from statistics.cc
 > >   as the cc file directly uses its definition (`Counter`).
 > The header file must be included from statistics.h as a definition
 > for class isc::auth::statistics::Counters requires
 > isc::statistics::Counter.
 > {{{#!cpp
 > 230 class Counters : boost::noncopyable {
 > 231 private:
 > 232     // counter for DNS message attributes
 > 233     isc::statistics::Counter server_msg_counter_;
 > }}}

 I know, but that's an indirect dependency.  Once we change the
 internal details of `Counters` it's possible the indirect inclusion
 will be gone.

 > > - why does `Counter` store the value as int?  Do we expect negative
 > >   values?  And are int (often 32-bit) enough (I don't think so btw)?
 > isc::statistics::Counter stores the value as 'unsigned' int. We don't
 > expect negative values. However, isc::data::IntElement only accepts
 signed
 > values (long) as I read the document for the class. I assume signed long
 in
 > most of environments can represent maximum value of unsigned int but
 > cannot of unsigned long. Also, wiki:StatsModule implicitly assumes that
 > counters are 32-bit according to MIB structure.

 Admitting it's beyond the scope of this task, I think (unsigned)
 32-bit integers are way too small for modern statistics counters.
 A 32-bit counter will overflow within a day if we increment it at the
 rate 100K/sec (and for high performance DNS servers 100Kqps is not
 that big).  We really need to revise the MIB definition, and extend
 related interfaces so we can handle 64 bit integers.

 > > - is this assumption correct?
 > > {{{#!cpp
 > >         // assume response is TSIG signed if request is TSIG signed
 > > }}}
 > I think yes, according to auth_srv.cc:
 > {{{#!cpp
 > 668     if (tsig_context.get() != NULL) {
 > 669         message.toWire(renderer_, *tsig_context);
 > 670     } else {
 > 671         message.toWire(renderer_);
 > 672     }
 > }}}

 This doesn't seem to be so (e.g.) in this case:

 {{{#!cpp
     } catch (const Exception& ex) {
         LOG_ERROR(auth_logger, AUTH_PROCESS_FAIL).arg(ex.what());
         makeErrorMessage(renderer_, message, buffer, Rcode::SERVFAIL());
         return (true);
     }
 }}}

 Some other comments on the revised version follow:

 - `MessageAttributes` constructor: it should now be exception free.
   I've updated the doygen comment.
 - `set/getRequestEDNS0` brief descriptions don't seem to make sense.
   especially for set, and for get it's not attribute"s" (plural).
   there are some other awkward brief descriptions.  Please re-read
   and fix them.
 - `MessageAttributes` get method: `const` isn't necessary for the
   parameters.
 - `Counters::get()`: AFAIC, return type should be able to be
   `ConstElementPtr`.

 - can't opcode_to_msgcounter, etc, be in unnamed namespace?
 - nit: why is this all capitalized?
 {{{#!cpp
     MSG_OPCODE_STATUS,   // Opcode =  2: STATUS

 }}}
   same for BADVERS.

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