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
-------------------------------------+-------------------------------------
Changes (by y-aharen):

 * owner:  y-aharen => jinmei


Comment:

 Hello,

 Replying to [comment:53 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.
 OK, I renamed them as per your suggestion (git ba2d4df).

 > 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);
 > }}}
 I updated the branch (git cddeabf).

 > - 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.
 I updated the branch to be BIND 9 compatible and added some notes and
 documentation (git 9430b06).

 > '''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.
 > }}}
 Corrected in the commit above.

 If they're OK, I'll merge the branch into master.

 Thanks,

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