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:  y-aharen      |                        Owner:
                Type:  enhancement   |  jinmei
            Priority:  medium        |                       Status:
           Component:  b10-auth      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  8             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:52 y-aharen]:

 > > What about the method name?  It still has the same misleading name.
 > > Naming may be ultimately a matter of taste, but I'd name it, e.g.
 > > `requestHasEDNS0`.  Same for other `getXXX` methods.
 > I'd prefer getXXX as requestHasXXX is confusing; I can't distinguish
 > whether it's a getter or setter. I propose getRequestHasEDNS0 and
 similar.

 `getRequestHasEDNS0` sounds awkward to me (and longish), but this
 point now seems to be quite minor and just a matter of taste.  It
 wouldn't make sense to spend more time on this.  I'd leave the final
 naming to you.

 The branch looks mostly okay, but I have a few last points to discuss:

 '''auth_srv.cc'''

 - opcode can be a reference, which would be slightly more efficient:
 {{{#!cpp
     const Opcode opcode = message.getOpcode();
     stats_attrs.setRequestOpCode(opcode);
 }}}
 - regarding opcode, there still seems to be subtle points.
   - it's not counted if it's a "response" (then silently dropped)
   - if it's TSIG signed and fails to validate, opcode isn't trustworthy

 In the end, the behavior may be okay as it is because 1) we can say
 "we only count statistics for 'requests'", 2) for both cases it's
 compatible with BIND 9 (in that, unless there's a specific reason for
 not doing so it's generally good to keep compatibility).  But I think
 these points should be documented, both as code comment for this part:
 {{{#!cpp
     const Opcode opcode = message.getOpcode();
     stats_attrs.setRequestOpCode(opcode);
 }}}
 and in the man page or guide or something.  We might also note that
 the opcode counter is the only one that could be counted regardless of
 the TSIG verification result.

 '''statistics.cc.pre'''

 - minor point, but opcode can also be missing when auth receives a
   response (see above)
 {{{#!cpp
     // Increment opcode counter only if the opcode exists; it can happen
 if
     // short message which does not contain DNS header received.
 }}}

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