BIND 10 #1613: auth per rcode statistics

[BIND 10 Development]

#1613: auth per rcode statistics
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:         |            Resolution:
  statistics                         |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  3
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 jelte]:

 The revised version basically looks okay.  Some final comments below.
 Any possible changes due to those comments will be very minor, so
 please feel free to merge it once you complete it.

 > > '''auth_srv.cc'''
 > >
 > > - Some cases are not counted, and some cases (where the response is
 > >   not generated) are incorrectly counted.  Former example is this:
 >
 > Ack, added a private method like your proposed wrapper.

 Maybe we'd implement it within the impl class to make sure it's
 inlined since this helper method is called for almost all requests and
 may be performance sensitive.  But maybe we can simply trust the
 compiler, so I'd leave it to you.

 > >   Also, we need to have tests that would fail without these
 > >   corrections.
 > >
 >
 > Naturally. I even added a way to test them more thoroughly than they
 were (also checks if for all counters are zero), perhaps we can do
 something similar for opcode, but we should probably wait on that possible
 refactor.
 >
 > BTW, not all paths are covered; I don't think there is currently a way
 to trigger the Exception catchall (the one that causes SERVFAIL after
 fromWire on auth_srv.cc:431). I tried to add a wiredata-mangling feature
 to our test system but then i discovered that all the actual parsing code
 at this point can only throw ProtocolErrors. Oh well.

 Mangled wire data are actually expected to result in FORMERR, so
 that's natural.  I guess if you make some kind of short buffer data in
 RDATA that triggers an exception in the input buffer class, then
 you'll see other types of exception.  But actually I think we should
 convert such cases to FORMERR (I should probably create a ticket), so
 trying to add a test for that case may not make sense anyway.

 We could even introduce a faked RR (RDATA) class and register it in
 the rrparamregistry (whose "from wire" constructor would throw
 isc::Unexpected or something) and have the auth server parse a message
 containing it, but that's probably too much for the purpose of this
 test.

 So, in conclusion, I'm okay with the current set of tests.

 > > '''others'''

 > Speaking of which, we should probably also have a ticket that allows you
 to do 'Stats show Auth opcode' (i.e. only show opcode, but all of them),
 and perhaps even a more general 'don't show zeroes'. Also looking forward
 to discussing more general ways to do statistics now, btw :)

 At least the first one seems to make sense.

 Some other minor points in the code:

 - in srv_test.cc, why did you change the indentation here?
 {{{#!c++
  void
  SrvTestBase::createRequestPacket(Message& message,
 -                                 const int protocol, TSIGContext*
 context)
 +                                    const int protocol, TSIGContext*
 context)
  {
 }}}
   The original indentation seems correct (per guideline) to me.
 - I made a few test methods const member functions.

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