BIND 10 #347: Implement query counters in auth module
BIND 10 Development
do-not-reply at isc.org
Thu Dec 9 10:01:41 UTC 2010
#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 your review!
Replying to [comment:5 jinmei]:
> '''general comments'''
> - Documentation: It's generally well written, but I'd like to see
> description about exceptions, that is, whether each method can
> throw an exception, and if yes, when.
OK. I described it.
> - If you don't do propset for $Id$, this line should be removed (in
> git, it will be meaningless "random" string, so we should probably
> deprecate this convention, at least for newly created files)
OK. I did propset.
> '''stats.h'''
> - I made some trivial (I think) editorial changes and commit it to
> the branch (r3689). Please check them.
I saw it. It seems to be good. Thank you.
> - The class name "Counter" sounds like representing a single counter
> and counter intuitive.
> - I'd like to see more in the class description, including how it
> generally works and how we are expected to use it.
> - Is this query specific? Except the counter names there doesn't
> seem to be anything specific to queries (or in that sense even
> specific to an authoritative server). If the intent is to extend
> the usage to other types of statistic counters eventually, it
> should be described accordingly.
"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.
> - setStatsSession(): I guess most of the points noted for
> AuthSrv::setXfrinSession() should apply.
OK. I described it.
> - getCallback(): I think this method naming is too restrictive, and
I'd
> suggest renaming it to, e.g., getSubmitFunc(). The reason is below.
> Whether it's used as a callback (e.g. from a timer) is a matter of
> caller, and the Counter class doesn't have to care about it. All the
> responsibility of this method is to return a callable functor object
that
> when called submit the collected statistics through the session channel
> (if provided via setStatsSession()). In fact, b10-auth could always
get
> the functor and submit the statistics by its own, right? (for example,
it
> may want to submit the final statistics before it shuts down). In
> general, if some public part of a class can be independent of how it
can
> be used, it's generally better to make that point explicit so that the
> class is more cohesive (and less coupled with its users).
>
> For the same reason, I think it's better to change the return type
to
> some more generic functor type (and I believe it's easy).
>
I almost adopted your next suggestion [comment:6].
> And for the same reason, it's okay (and useful) to mention its
> intentional usage as a hint, but it should also be described that it
can
> be used in other ways.
OK. I described it.
>
> '''stats.cc'''
> - why do we need a separate namespace? Just following what asio_link
> did? If so, the intent was to make it a separate library belonging
> to a separate namespace eventually. Unless the stat module has the
> same intent, I'd suggest not having the separate namespace for now
> (we should probably need a common "auth" namespace, although that's
> a different topic)
OK. I removed "statistics" namespace and will add common namespace
"auth" in the future. It was described as TODO.
> - naming: it would be better to use consistent terminology, i.e. if
> we need and use a namespace named "statistics", the
> header/implementation
> files should better be called "statistics.{h,cc}", and vice versa.
> I personally prefer (especially in C++) complete words such as
> "statistics" rather than the geeky abbreviation like "stats", but
> it's generally a matter of taste.
> (BTW: the redundancy is also related to the question of why we need
> a separate name space.)
OK. I renamed the file name to "statistics.{h,cc}".
> - CounterImpl declarations: getCallback() and sendStats()
> could/should be const member functions.
OK. I did it.
> - CounterImpl constructor: it seems ad hoc to pass "verbose_mode"
> this way. It's also not a good practice in that it ties the
> Counter(Impl) class with the AuthSrv class through an implicit
> interface (i.e. not via public methods). I can understand if this
> is a short term workaround until we have a well defined logging
> framework, however. If so, please explicitly note this in the
> doxygen comment. If not, and you think this is a good technique,
> we need to discuss it because I'd strongly disagree:-)
I agree with you. I noted it as TODO.
> - CounterImpl constructor: why dynamically allocating counters_? I
> don't see the need for it.
OK, it doesn't need to be. I modified it to allocate statically.
> - CounterImpl::sendStats()
> - CounterImpl::sendStats() try-catch part
> - group_sendmsg() (not recv) can also throw. should this be caught
> or ignored here? probably it should, because we wouldn't like to
> kill b10-auth just due to some communication failure (though it
> may be quite rare). In this sense, I suspect the comments should
> be positioned closer to the group_recvmsg() call.
> - group_recvmsg() can throw (at least) two types of exceptions:
> SessionTimeout when it times out; SessionError for other errors,
> including when receiving bogus data. I guess we'd like to catch
> both for the same reason as that for group_sendmsg().
OK. I modified the code to catch these exceptions as suggested below.
> - The comment and log messages assume that the other end is the
> b10-stats program, but interface-wise it's not necessarily correct
> (that could be a third-party app that implements the stat API,
> right?)
I agree with your reply in [comment:9 jinmei].
> - Considering these, my suggestion of this part is as follows:
Thank you. It seemed be good suggestion. I adopt it.
> {{{
> // group_{send,recv}msg() can throw an exception when encountering
> // an error, and group_recvmsg() will throw an exception on
timeout.
> // We don't want to kill the main server just due to this, so we
> // handle them here.
> try {
> const int seq =
> stats_session_->group_sendmsg(set_stats, "Stats");
> if (verbose_mode_) {
> std::cerr << "[b10-auth] send statistics data" <<
std::endl;
> }
>
> // TODO: parse and check response from the statistics
recipient;
> // in case of b10-stats, currently it just returns empty
message.
> isc::data::ConstElementPtr env, answer;
> stats_session_->group_recvmsg(env, answer, false, seq);
> } catch (const isc::data::SessionError& ex) {
> if (verbose_mode_) {
> std::cerr << "[b10-auth] "
> << "timeout happened while sending statistics
data:
> "
> << ex.what() << std::endl;
> }
> } catch (const isc::data::SessionTimeout& ex) {
> // catch the exception and do nothing.
> if (verbose_mode_) {
> std::cerr << "[b10-auth] "
> << "communication error in sending statistics
data:
> "
> << ex.what() << std::endl;
> }
> }
> }}}
> - BTW, even if it doesn't cause timeout and exception, we should
> actually not even block in group_recvmsg (and also
> group_sendmsg(), which can happen because it's a connected
> channel): b10-auth shouldn't stop handling DNS queries even for 1
> second. Since it should relatively a rare event we can live with
> the current blocking mode (and there are actually other parts of
> b10-auth that have the same problem). But please leave a TODO
> comment on this point. We should eventually solve it.
I agree with your reply in [comment:9 jinmei]. I wrote a TODO comment
which
describes the problem.
> - CounterImpl::sendStats(): the "catch" case doesn't seem to be
> covered by the tests
OK. I wrote new tests in statistics_unittest.cc.
> - Counter::getCallback(): this could/should be a const member
function.
OK. getCallback() no longer exists.
Replying to [comment:6 jinmei]:
> Comment(by jinmei):
> Replying to [comment:5 jinmei]:
>
> Self follow-up:
>
> > - getCallback(): I think this method naming is too restrictive, and
I'd
> suggest renaming it to, e.g., getSubmitFunc(). The reason is below.
> >
> On further look, it doesn't even seem to have to be a functor. We
could
> define a callback exactly where it's used, i.e., (in the current
> implementation) in main.cc, and other methods could just be
> straightforward normal functions:
>
> in main.cc
> {{{
> void
> statsTimerCallback(AuthSrv* server) {
> // we may have to care the case where 'server' is invalid
> server->submitStatistics(); // instead of getStatsCallback()
> }
> ...
> itimer->setupTimer(boost::bind(statsTimerCallback,
auth_server),
> STATS_SEND_INTERVAL_SEC);
> }}}
>
> AuthSrv::submitStatistics() would simply call
counter->submitStatistics()
> (renamed from getCallback()), and Counter::submitStatistics() would do
> what CounterImpl::sendStats() currently does.
>
> IMO this is a much better design.
I agree with you. I adopted it.
Replying to [comment:7 jinmei]:
> '''general'''
> - The Counter class should have its own tests.
OK. QueryCounters (renamed from Counter) has its own tests in
statistics_unittest.cc.
> '''auth_srv.h'''
> - I don't think this have to be a doxygen comment:
> {{{
> /// for the typedef of asio_link::IntervalTimer::Callback
> }}}
> - BTW, if we change the callback scheme as I suggested, we don't
> have include asio_link.h/stats.h. (Counter can be a forward
> declaration, and shouldn't be here anyway. see below)
> - Basically up to you, but this comment seems trivial and I wouldn't
> bother to mention it:
> {{{
> #include <auth/stats.h>
> }}}
OK. As suggested above I changed the callback scheme and these includes
are not necessary now.
> - getStatsCallback(): see the callback scheme discussion.
OK. This function no longer exists.
> - Member variable "counter" breaks the spirit of pimpl. The comment
also does not
> seem to make sense to me. Whether it's incremented in a member
> function of AuthSrv or AuthSrvImpl, "counter" can be defined in
> AuthSrvImpl. The current implementation also is exception unsafe
> due to this variable. See below. In any case "counter" should be
> hidden in impl_. BTW, to be consistent about naming, I'd rename
> it to "counter_".
OK. I renamed it to "counters_" and it is now inside AuthSrvImpl.
> '''auth_srv.cc'''
> - AuthSrv constructor: this is not exception safe. If the allocation
> of counter fails memory for impl_ will leak. Due to this, and for
> other reasons also, counter should in impl_. BTW: it also doesn't
> make sense to me to dynamically allocate Counter, especially
> if/when we hide it in .cc. Unless this is optional (in the current
> implementation it's not) AuthSrvImpl can simply contain a Counter
> object.
OK. I modified it to allocate statically (and it is now in AuthSrvImpl).
> - 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.
> - AuthSrv::getStatsCallback(): the comment seems to me so trivial
> that I wouldn't bother to mention it:-)
OK. AuthSrv::getStatsCallback() no longer exists.
> '''auth.spec.pre.in'''
> - as pointed out before, it's not necessarily "b10-stats":
> {{{
> + "command_description": "Send statistics data to b10-stats at
once",
> }}}
> I'd say, e.g., "...to a statistics module at once".
OK. I almost adopt it.
> '''asio_link.h'''
> - I'd like to see more description for the IntervalTimer class,
> including how it generally works and how it should be used (sample
> code may help).
OK. I described sample code and how it works.
> - I don't understand this statement:
> {{{
> +/// Copy of this class is prohibited not to call the callback function
twice.
> }}}
> Could the callback function be called twice if we allowed copy? (I
> don't object to disabling the copy per se). BTW, being pedantic,
> we don't copy a "class"; we copy an object (or instance) of a
> class.
I misunderstood, it can't be called twice. I removed the comment.
But I leave the class non-copyable.
> - IntervalTimer constructor: passing (and exposing as a result)
> asio::io_service(&) breaks the intent of this wrapper and is not a
> good practice. The only place (as a short-to-middle term
> workaround) we can see it is in the cc::Session constructor. No
> other code except that within the asio_link wrapper should directly
> refer to it, even as a reference. Please update the interface so
> that it takes IOService(&) instead of asio::io_service(&). I
> believe you can do it with a trivial change.
I agree with you. The constructor now receives a reference to IOService.
> - IntervalTimer destructor: it should explain it cancels a running
> timer. BTW: what happens if we destruct IntervalTimer before
> starting a timer?
I thought it is required to explicitly cancel the timer before
destruction.
I removed the code to cancel the timer because it is cancelled
in the destruction of deadline_timer (I wrote a comment).
> - setupTimer(): this should explain the expected signature of
> cbfunc. See, for example, the asio documentation:
> http://think-
async.com/Asio/asio-1.4.5/doc/asio/reference/basic_deadline_timer/async_wait.html
> It should also explain it doesn't only register the callback but
> also actually starts the timer.
OK. I described them.
> - setupTimer(): should it return a (boolean) value? It doesn't seem
> to return no other value than true. If so, it's better to make it
> void.
OK. I made it void.
> '''asio_link.cc'''
> - It'd be helpful if IntervalTimerImpl could have more description.
> See other pimpl classes defined in asio_link.cc.
I don't see much description in *Impl classes in asio_link.cc, but
I added some comments to IntervalTimerImpl.
> - IntervalTimerImpl::setupTimer(): IMO this use of assert is wrong.
> In this case we should use exception, and document the restriction,
> and test the case.
OK. I modified to throw the exception, documented the restriction
(interval should be greater than 0, call back function should not
be empty), and wrote a test case.
> - IntervalTimerImpl::callback(): Likewise, this assert seems to be
> wrong because, if I understand the code correctly, the application
> can (incorrectly) pass an empty functor via setupTimer(). If want
> to make sure this doesn't happen, we should require it at the time
> of setupTimer(), throw an exception otherwise, document it, and
> test it.
OK. IntervalTimerImpl::callback() no longer exists. setupTimer() checks
its arguments.
> '''asio_link_unittest.cc'''
> - doTimerTest() doesn't seem to have to be in the ASIOLinkTest
> class (because it's small and is only used by a single test).
> Placing it in a different place makes the code unreadable.
OK. I moved the codes into TEST_F(ASIOLinkTest, startIntervalTimer).
doTimerTest() no longer exists.
> - doTimerTest(): I'd like to make sure the timer expires with the
> given interval, although it could be optional.
OK. Some tests requires the timer expires with the given interval, so
I wrote a test case. The test allows interval has a mergin
(currently +/- 50 milliseconds).
> - timerCallBack() doesn't have to be a separate function (it can be
> embedded in TimerCallBack::operator())
OK. I embedded the codes to TimerCallBack::operator()().
> - ASIOLinkTest::startIntervalTimer(): why is itimer allocated
> dynamically. It makes (at least in theory) the code exception
> unsafe, and we can actually simply avoid the dynamic allocation:
> {{{
> IntervalTimer itimer(io_service_->get_io_service());
> doTimerTest(&itimer);
> }}}
> btw, you can omit the namespace "asio_link::" because we declare
> 'using namespace' for it.
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?
> - I'd like to see more test cases, including
> - Invalid arguments (see the comments on IntervalTimerImpl)
> - what happens if we delete the timer
> - call setupTimer multiple times, with different arguments
OK. I wrote some test cases: invalidArgumentToIntervalTimer,
destructIntervalTimer and overwriteIntervalTimer.
> '''auth_srv_unittest.cc'''
> - as commented, we need a separate test class for Counter (shouldn't
> be "in future", especially because the tests are incomplete - see
> below).
OK. I wrote a seperated test case for QueryCounters.
> - AuthSrvTest::sendStatsWithoutSession() This is not a good unit test
> because a human needs to examine the output. Modify the interface
> and use EXPECT_xx.
OK. AuthSrvTest::sendStatsWithoutSession() no longer exists.
I modified submitStatistics() to return bool and test it is 'true'.
> - AuthSrvTest::counter_incUDP/TCP(): these don't do any tests. In
> terms of auth server, we should test by sending a "fake" UDP/TCP
> message and see if the corresponding counter is incremented. For
> the more focused tests of Counter, we directly increment the
> counters and check the results. For that matter, we'll need to an
> inteface to retrieve the specified counter value in the Counter
> class (which would be good anyway). BTW, using _ in the test name
> effectively breaks our naming rule. I'd rename it to
> inc[rement]CounterUDP, etc. Same comment applies to some other
> cases.
OK. I renamed them and wrote some test cases to adopt your suggestion.
> - AuthSrvTest::counter_sendStatsWithSession(): this test is
> insufficient. We should define an appropriate MockSession, which
> intercepts the submitted message, parses and stores it, and then we
> should check the submitted values are correct using EXPECT_xxx.
OK. I wrote a test case that parses and examines the submitted message.
> '''main.cc'''
> - my_command_handler()
> - in this context auth_server shouldn't be NULL. I'd rather assert
> it or assume it's valid.
OK. I added a code to assert it is not NULL.
> - main()
> - I have comments/suggestions about timer callbacks. See above.
OK. I modified as you have suggested.
> '''ChangeLog'''
> - Shouldn't Kambe-san be listed as a contributor, too? (Just
checking.)
He says not.
> - It would be nice if it could briefly explain how we can see the
> collected statistics (via bindctl perhaps).
OK. I added a part of the result of the command 'sendstats'.
Thank you again for your great feedback.
--
Ticket URL: <http://bind10.isc.org/ticket/347#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list