BIND 10 #1613: auth per rcode statistics

[BIND 10 Development]

#1613: auth per rcode statistics
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  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 jelte):

 Replying to [comment:10 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.
 >
 > >
 > > 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.
 >

 oh, good idea, done.

 > > >   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.
 >

 given the way it works now, I can't see a way to get the servfail other
 than a bug in our code (in which case servfail is fine imo). If we didn't
 already parse the header earlier, a 10-octet packet might trigger it, but
 we do parse the header earlier.

 > 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.
 >

 yes, i think that would be overdoing it a bit :) (the only purpose imo
 would be to get more code coverage, really)

 > 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.
 >

 my reasoning behind the second one was that, for instance, if you don't
 run ddns, a lot of rcodes are irrelevant (and should be 0 anyway), so you
 may not be interested in them. But maybe there are better ways to achieve
 that. Either way, IMO the list of items is already getting too large to
 usefully do 'show them all' on the command line.

 > 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.

 oh, sorry, artifact of changing it to use the mangler, and not reverting
 it completely. Fixed.

 > - I made a few test methods const member functions.

 thanks.

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