BIND 10 #191: Implement a initial version of stats

[BIND 10 Development]

#191: Implement a initial version of stats
-----------------------------+----------------------------------------------
      Reporter:  naokikambe  |        Owner:  jinmei              
          Type:  task        |       Status:  reviewing           
      Priority:  major       |    Milestone:  y2 6 month milestone
     Component:  statistics  |   Resolution:                      
      Keywords:              |    Sensitive:  0                   
Estimatedhours:  20.0        |        Hours:  0                   
      Billable:  0           |   Totalhours:  3.33                
      Internal:  0           |  
-----------------------------+----------------------------------------------
Changes (by naokikambe):

  * owner:  naokikambe => jinmei
  * estimatedhours:  0.0 => 20.0


Comment:

 Thank you, Jinmei-san,

 Replying to [comment:6 jinmei]:
 > Please also provide a proposed ChangeLog entry for this ticket.
 I committed at r2845

 Replying to [comment:4 jinmei]:
 >  I'd say at least 80% of it should be covered before doing formal review
 by a third person.
 I committed at r2846

 Replying to [comment:11 jinmei]:
 > I've not completed my review, but am providing some initial feedback:
 >
 > '''run_b10-stats.sh.in'''
 >  - is B10_FROM_BUILD necessary? (the same comment applies to other .sh)
 Yes, it's not necessary. I removed them. r2847

 > '''stats.spec.pre.in'''
 >  - I don't think this is a good "default". (it rather looks like an
 "example")
 > {{{
 >         "item_default": "123abc at xxxx",
 > }}}
 This item is not optional. I'm sure that this value is not appropriate. I
 changed this item to no default value. r2848

 > '''unittest_fakesession.py'''
 >  - this seems to be largely derived from
 config/tests/unittest_fakesession.py.  please note it if you do something
 like that.  it will help reduce review burden very much.  also, it's even
 better to unify the code (with appropropriate abstraction if per-module
 customization is necessary).  having duplicate code is generally not a
 good practice.
 Yes, I copied this whole file from here. You're right. I created a whole
 new class which is mock-up classes against isc.cc.session and
 isc.config.ccsession. r2862 And I removed unittest_fakesession.py under
 this directory. r2865

 >   moreover, I'm not even sure about the purpose of this method.  Is this
 while-select trick necessary?  Please describe the purpose of this method
 and how it's expected to be used (as comments).
 >  - this line in close() seems to be dead code:
 > {{{
 >                 raise Exception('Unexpected return values from
 select():', r, w, e)
 > }}}
 In this new mock-up classes, I don't use this tricky way.

 >  - test_boss_stub: I don't this is a good test because get_datetime()
 may return different values in the test and in the testee.  (code
 duplication of get_datetime() is another bad thing)
 I have recognized this before but I couldn't find the best way to resolve
 it. I rewrote the codes so that the same values are returned between test
 code and target code. I created a new mock-up classes fake_time.py against
 time.py which is standard class of python. Also I aggregated the
 definitions of ''get_datetime()'' and ''get_timestamp()'' in
 ''stats.py.in''. r2864

 > '''b10-stats_stub_test.py'''
 >  - I'd move tearDown() right after setUp().
 OK. I moved tearDown() up. r2864

 >
 > '''stats_stub.py.in'''
 >  - BossModuleStub.send_command(): the method name is too generic for
 what it's actually doing.
 I changed the name of the methods to the followings according to actual
 doing:
 Before::
 {{{
 #!python
   BossModuleStub.send_command()
   AuthModuleStub.send_command_udp()
   AuthModuleStub.send_command_tcp()
 }}}
 After::
 {{{
 #!python
   BossModuleStub.send_boottime()
   AuthModuleStub.send_udp_query_count()
   AuthModuleStub.send_tcp_query_count()
 }}}

 r2864

 >  - I suggest all temporary resources be cleaned up in tearDown().  from
 a quick look, it seems self.listener has to be stopped.
 OK. I did that. I added the codes which stops the listener object and the
 subject object into each ''tearDown()'' method.
 r2864

 > '''b10-stats_test.py'''
 >  - the intent of this part is not clear.  please add comment:
 > {{{
 >         for i in (0, 1):
 >             self.session.get_message("ConfigManager", None)
 > }}}
 This codes isn't unnecessary because I created a mock-up class against
 ''ConfigManager''. I removed this codes.

 Replying to [comment:12 jinmei]:
 > I also have some high level questions about the design of stats.py:
 I don't have strong opinions but it might be wrong ways to use these
 design patterns. :(

 >  - I don't see the need for the abstraction using the observer pattern
 because there is only type of listener (=observer).  what's the background
 rationle of this design decision?  Please describe it in more detail in
 the source code.
 I added a comment to the code like this. In the initial release, I'm also
 sure that observer pattern isn't definitely needed because the interface
 between gathering and reporting statistics data is single.
 However in the future release, the interfaces may be multiple, that is,
 multiple listeners may be needed. For example, one interface, which stats
 module has, is for between ''config manager'' and stats module, another
 interface is for between ''HTTP server'' and stats module, and one more
 interface is for between ''SNMP server'' and stats module. So by
 considering that stats module needs multiple interfaces in the future
 release, I adopted the observer pattern in stats module. But I don't have
 concrete ideas in case of multiple listener currently.
 r2864

 >  - Likewise, I was not sure about the intent of the seeming introduction
 of singleton.  From a very superficial look at the code the intent seems
 to make SessionSubject a singleton class, but at least in the usage of
 stats.py we don't have to bother to have such a tricky technique, because
 there cannot be more than one SessionSubject object in the first place.
 Besides, I suspect this implementation actually doesn't realize what is
 seeminly intended:
 > {{{
 > class SessionSubject(Subject):
 >     """
 >     A concrete subject class which creates CC session object
 >     """
 >     __metaclass__ = Singleton
 > }}}
 I added a comment to the code like this. At the beginning of coding, one
 UNIX domain socket is needed for config manager, another socket is needed
 for stats module, then stats module might need two sockets. So I adopted
 the singleton pattern because I avoid creating multiple sockets in one
 stats module.
 But in the initial version stats module reports only via bindctl, so just
 one socket is needed. To use the singleton pattern is not important now.
 :(
 r2864

 >   according to a python 2-to-3 guide, this syntax has been depreciated:
 http://diveintopython3.org/porting-code-to-python-3-with-
 2to3.html#metaclass
 >   Please clarify the intent of the code in more detail.
 Thank you for your giving reference. I didn't know this guide. I rewrote
 the code according to the guide.

 Are above all my answers just enough? Please be free to ask again if you
 can't understand that.

 Thanks,

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