BIND 10 #347: Implement query counters in auth module

[BIND 10 Development]

#347: Implement query counters in auth module
------------------------------+---------------------------------------------
      Reporter:  naokikambe   |        Owner:  jinmei               
          Type:  enhancement  |       Status:  reviewing            
      Priority:  major        |    Milestone:  y2 12 month milestone
     Component:  b10-auth     |   Resolution:                       
      Keywords:               |    Sensitive:  0                    
Estimatedhours:  0.0          |        Hours:  0                    
      Billable:  1            |   Totalhours:  0                    
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by y-aharen):

  * owner:  y-aharen => jinmei


Comment:

 Jinmei-san, thank you very much for you review.

 Replying to [comment:12 jinmei]:
 > Replying to [comment:10 y-aharen]:
 >
 > Overall, the revised branch looks much better.  I have some non
 > trivial comments though, which are below.
 >
 > '''general comment'''
 >  - I made some trivial suggested changes (r3904).  Please check them
 >    out (and override any of them if necessary)
 I've checked them and they looks good. Thank you.

 >  - While I personally prefer "statistics" over "stats", consistency is
 >    even more important.  Right now there is a mixture of "xxxStatsyyy"
 >    and "xxxStatisticsyyy" (e.g. setStatsSession() vs
 >    submitStatistics()), which is not good.  If we name something
 >    "xxxStatistics", everything else should be named with "Statistics"
 >    (not stats); if we name it "xxxStats", others should also be named
 >    with "Stats".  The choice is ultimately yours, but it should be
 >    consistent.  I've fixed one obvious issue with this regard directly
 >    in the branch: header guard definition (__STAT(ISTIC)S_H).
 Sorry, I forgot to change some name. I changed the names as you suggested.

 > '''statistics.h'''
 >  - now there's no need to include boost/function.hpp.  I've removed
 >    it.
 OK, it can be removed.

 >  - as for the class name:
 > > "Counter" class holds a set of query counters. Thus, I renamed it to
 > > "QueryCounters". Is it OK? Currently it holds only "UDP" and "TCP"
 > > type, but we may extend other types like "Notify", "A/Ixfr" type and
 > > so on in the future. I described it as a comment in the code.
 > >
 > My concern is that with the intent of extension we'll soon encounter
 > the need for renaming the class.  Note that notify/ixfr/update are not
 > even "queries" (to be very accurate about terminology); we may want to
 > have other statistics counters that may not be related to DNS protocol
 > handling (e.g, we may want to have a counter about the number of
 > reloads).  I don't know the future plans about these possibilities,
 > but unless we want to define separate classes for these different
 > types of statistics parameters, I'd choose a more generic name such as
 > AuthCounters (but note also that as I pointed out in my previous
 > comment it may not even be specific to the authoritative server; we'll
 > naturally consider having the same framework for b10-recurse, for
 > example.  So, eventually, we may want to have a separate library
 > module src/lib/statistics or something like that, where we define a
 > most generic "Counters" class that can be used for any kinds of
 > counter based statistics).
 >
 > But at the same time, there's a risk of premature overabstraction, so
 > it's a tradeoff issue.  The choice is basically up to you, but if I
 > were the principal author, I'd begin with "AuthCounters" with a note
 > that we may eventually want to make it a separate library with a
 > higher abstraction level (with the cost of renaming classes and its
 > overhead).
 I think the name 'AuthCounters' is suitable for the class, so I renamed
 it. I also noted the design of the class may change in the future, not
 only holding counter values nor specific to Auth.

 > '''statistics.cc'''
 >
 >  - getCounters() (effectively) exposes an internal implementation
 >    details (i.e., that the counters are stored in std::vector), which
 >    is not a good practice.  I'd prefer passing a counter type and
 >    retrieving that specific counter.  BTW: why "shouldn't this be
 >    called except from tests" as documented in the header file?  It may
 >    not be of much use in general, but I don't see the reason for
 >    prohibiting it.  And, in any case, restricting usage by comment is
 >    not a reliable way to do so.
 To pass 'QueryCounterType' we need definition of it. AuthSrv provides an
 interface to getCounters() so I've added '#include <auth/statistics.h>'
 into auth_srv.h and modified getCounter() to return single counter value
 of specified type.

 >  - QueryCountersImpl::inc(): is there any reason for converting
 >    std::out_of_range to InvalidParameter?  On one hand that's
 >    generally a good practice, but we basically allow standard
 >    exceptions to be propagated through our public interfaces (unlike
 >    boost exceptions, which we generally try to hide), so we don't have
 >    to do anything special here.  If there's a good reason to catch
 >    (and convert) the exception internally, we should catch it by
 >    reference, not as an object.
 I removed try-catch which converts std::out_of_range to
 InvalidParameter.QueryCountersImpl::inc() throws std::out_of_range.

 > > > '''auth_srv.cc'''
 >
 >  - AuthSrv::processMessage
 > > >  - AuthSrv::processMessage(): in which case we can see getProtocol()
 > > >    be neither UDP nor TCP?  In general, I don't like to handle an
 > > >    impossible case this way.  I'd either throw an exception or even
 > > >    assert() here.
 > > I agree with you. I modified to throw an exception.
 > >
 > Okay, but the exception case isn't tested.  Please also add
 > documentation about the exception in auth_srv.h (to make it better
 > gradually).
 I wrote test case and added documentation about the exception.

 >  - AuthSrv::processMessage another point: this method handles all
 >    incoming requests including notify and IXFR, not just normal
 >    queries.  So, technically, incrementing a "query" counter here is
 >    not correct.  See also the discussion about class naming above.
 OK. I moved the code to a function and it is called from
 AuthSrvImpl::processNormalQuery() and AuthSrvImpl::processAxfrQuery().

 >  - AuthSrv::submitStatistics: this should be a const member function.
 OK. I made it const member function.

 > '''asio_link.h'''
 >  - about the IntervalTimer class description: I'm not sure whether we
 >    need this note:
 > {{{
 > /// This class is mainly designed to use for calling
 > /// \c QueryCounters::submitStatistics() periodically.
 > }}}
 >   What's the purpose of this note?  The interface of interval timer
 >   seems quite generic and I don't see the need for mentioning the
 >   "main purpose" here.
 Just to describe why I wrote this class. I reconsider it and I think the
 comment should not be there (it is also described in ChangeLog). I removed
 it.

 >  - IntervalTimer constructor
 >   - We should document that "cbfunc" shouldn't be an empty functor.
 I added documentation.

 >   - Can it throw asio::system_error?  I was not sure how that could
 >     happen, but if it really happens:
 >     - we should catch the exception internally and convert it to our
 >       own one (we need to hide any asio specific definitions, which is
 >       the purpose of this wrapper in the first place)
 >     - we should cover that case in tests.
 I misunderstood the asio::deadline_timer library. The constructor won't
 throw asio::system_error. I removed the documentation.

 >  - IntervalTimer destructor: my previous question doesn't seem to be
 >   addressed in the documentation: "what happens if we destruct
 !IntervalTimer before starting a timer?"
 If we destruct it before starting a timer, the callback function should
 not be called. I wrote a test.

 >  - setupTimer(): like the constructor, asio::system_error shouldn't be
 exposed.
 I wrote try-catch to convert asio::system_error to isc::Unexpected and
 added documentation. It is hard to modify the library to throw an
 exception, so I didn't wrote a test.

 > '''asio_link.cc'''
 >  - IntervalTimerImpl::callback()
 > > OK. IntervalTimerImpl::callback() no longer exists. setupTimer()
 checks
 > > its arguments.
 >
 > I guess you mean "no longer exits (or asserts)"?  BTW with the check
 > in setupTimer() I'm okay with the assert() (or may even be a good
 > practice, but it's up to you).
 It means I removed assertion from IntervalTimerImpl::callback(). I think
 throwing an exception is suitable for parameter check, so the code throws
 an exception.

 > '''asio_link_unittest.cc'''
 >  - as a general comment, it looks like the added tests make
 >    ASIOLinkTest unnecessarily big(ger).  ASIOLinkTest is already
 >    pretty complicated due to perform some tricky tests using actual
 >    socket I/O, while the timer tests don't need anything of these.  It
 >    would be much better to define a different test fixture (say,
 >    IntervalTimerTest) and enclose all timer related stuff there.
 I added a test fixture 'IntervalTimerTest' (partially copied from
 ASIOLinkTest). All of all timer related stuff are now in it.

 >  - there's no need to construct an IntervalTimer::Callback object
 >    explicitly thanks to the magic of boost::function.  e.g., instead
 >    of:
 > {{{
 >                 timer_.setupTimer(IntervalTimer::Callback(
 >                                       TimerCallBack(test_obj_)), 1);
 > }}}
 >   we could say:
 > {{{
 >                 timer_.setupTimer(TimerCallBack(test_obj_), 1);
 > }}}
 >   This will help make the code simpler and more readable.
 I modified the code as you suggested. It is much easier to read.

 >  - ASIOLinkTest::startIntervalTimer()
 > > OK. Almost all of IntervalTimers are now allocated statically, except
 in
 > > destructIntervalTimer. In the test code we have to delete it.
 > > I enclosed it by ASSERT_NO_THROW, is that OK?
 > >
 > It doesn't address the concern about the exception safety.  It's not
 > about the case where newing the timer throws an exception.  It's about
 > an exception triggered after the new before the test terminates.  One
 > way to make it 100% correct would be to use a smart pointer for the
 > itimer, but that would be probably too much in this context; if the
 > test throws unexpectedly, the whole program would soon exit anyway, so
 > the perfect exception safety wouldn't be worth the code complexity.
 > After making the itimers real object whenever possible, my suggestion
 > is to ignore the exception safety problem with an explicit comment
 > like: "this code isn't exception safe, but we'd rather keep the code
 > simpler and more readable as this is only for tests and if it throws
 > the program would immediately terminate anyway".
 >
 > BTW, whether or not adopting this suggestion, these _NO_THROW()s don't
 > make sense anyway.  I'd remove them.
 I removed ASSERT_NO_THROW and added comment as you suggested.

 >  - the additional tests introduce another 10 seconds of delay.  in
 >  general, imposing a long delay in unit tests is not a good practice
 >  as unit tests are supposed to be performed "instantly".  These types
 >  of delay have been introduced in our tests already (some of them were
 >  introduced by myself), and so it's not specific to the timer tests,
 >  but 10 seconds still look quite substantial.  I'd not necessarily
 >  request it be solved right now, but the timer should eventually have
 >  finer granularity for the timer duration and use shorter timer
 >  periods for tests.  For the moment I suggest adding some notes about
 >  this point in comments.
 I noted these tests takes long time and this issue should be addressed in
 future.

 > '''statistics_unittest.cc'''
 >   - in QueryCountersTest() verbose_mode_ should be initialized to false
 >   explicitly.  Otherwise, it could cause noisy log messages during
 >   tests depending on the environment (it happened to me).
 OK. verbose_mode_ is initialized to false.

 >   - incrementUDPCounter: I'd test if the counter is initially 0, and
 >     if it's really incremented by one after counters.inc().  Same for
 >     incrementTCPCounter, AuthSrvTest::queryCounter{UDP,TCP}.
 I added some EXPECTs to these tests to check the counter is initially 0,
 and the value is 1 after counters.inc().

 > '''ChangeLog'''
 >  - I couldn't play with the statistics using bindctl just from this
 >  description.  After understanding how it works, I'd suggest revising
 >  the text as follows:
 > {{{
 >   TBD.  [func]                y-aharen
 >       src/bin/auth: Added a feature to count queries and send counter
 >       values to b10-stats periodically. To support it, added wrapping
 >       class of asio::deadline_timer to use as interval timer.
 >       The counters can be seen using the "Stats show" command from
 >       bindctl.  The result would look like:
 >         ... "auth.queries.tcp": 1, "auth.queries.udp": 1 ...
 >       Using the "Auth sendstats" command you can make b10-auth send
 >       the counters to b10-stats immediately.
 >       (Trac #347, svn rxxxx)
 > }}}
 >   (I used b10-stats in this context instead of generic "statistics
 >   module" because it would be more helpful for the users)
 OK, I changed the text as you suggested.

 Thank you again for your review.

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