BIND 10 #2225: Implement counters into Xfrout (3/3)

[BIND 10 Development]

#2225: Implement counters into Xfrout (3/3)
-------------------------------------+-------------------------------------
            Reporter:  naokikambe    |                        Owner:  muks
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  xfrout        |                    Milestone:
            Keywords:                |  Sprint-20121218
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  7             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by naokikambe):

 * owner:  naokikambe => muks


Comment:

 Hello, thank you for reviewing.

 Replying to [comment:28 muks]:
 > We decided during the daily status call yesterday that first we'll first
 review just the `trac2225_statistics` branch. After it is reviewed and
 merged, the other branch `trac2225_xfrout` can be reviewed, either in this
 ticket or as part of another ticket if required. So, here is my first
 review of the `trac2225_statistics` branch.

 I agreed. Let's focus on reviwing `trac2225_statistics`.

 > First, I don't understand if `Counter` is expected to behave somewhat
 like a singleton, or I don't follow why some code is written the way it is
 (the `_Statistics` class, and some `Counter` initialization outside
 `__init__()`). Is it expected to create equivalent `Counter` objects,
 based on what spec file is passed? Suppose that you have the following
 code:
 > {{{
 >   from isc.statistics import Counter
 >   counter1 = Counter("/path/to/foo.spec")
 >   counter2 = Counter("/path/to/bar.spec")
 >   counter3 = Counter("/path/to/foo.spec")
 >   counter1.clear_counters()
 > }}}
 >
 > Are `counter1` and `counter3` supposed to be equivalent? They aren't
 based on the current code, but are they intended to be equivalent? With
 the last statement in the block of code above, are counters in `counter3`
 also supposed to be cleared? (I think they are not, currently). If such
 equivalence is desired, it's better to rewrite it such that there is a
 factory method in the `counter` module, that returns a `Counter` object
 based on the passed spec file.
 >
 > I am wondering about the above, because a singleton was in use before
 and I want to know what the intention was.

 In one bind10 module, it is NOT indented that the different spec files are
 read in the above way.  Maybe I think counters in `counter3` would be
 cleared by ``counter1.clear_counters()``.

 However in different modules as followings, it is indented that each spec
 file is read individually. That is, ``counter1.clear_counters()`` doesn't
 clear counters in ``counter2``, and vice versa.

 Module Foo (b10-foo):
 {{{#!python
 from isc.statistics import Counter
 counter1 = Counter("/path/to/foo.spec")
 counter1.clear_counters()
 }}}

 Module Bar (b10-bar):
 {{{#!python
 from isc.statistics import Counter
 counter2 = Counter("/path/to/bar.spec")
 counter2.clear_counters()
 }}}

 So if we should consider such equivalence in the above former case, I also
 think we would introduce a factory method or something as you mentioned.
 Sorry, please suggest whether we should or not. If yes, I'd like to stop
 reviewing and try to redesign the interface of the class.

 > Assuming that the code is as-is, the following are my comments:
 >
 > * I suggest renaming the class to the plural form (`Counters`) as it
 manages multiple counters.

 Ok, I've renamed:
 {{{
 d4e066a [trac2225_statistics] rename the class to the plural form
 (`Counters`) as it manages multiple counters
 }}}

 > * The module's documentation says that `init()` must be called. Where is
 `init()` defined?

 Not defined. I've corrected documentation.
 {{{
 6a4b958 [trac2225_statistics] correct documentation about an undefined
 method
 }}}

 > * Some methods are declared at static scope to the module, rather than
 as a part of `Counter` class itself. We can indicate them as private by
 prefixing an underscore. Is there a reason why `Counter.inc()` calls
 `_inc_counter()` and not `self._inc_counter()`?

 Because we don't need to relate `_inc_counter()` with `self`, I did so.
 There is no reason other than this.

 > * `Counter` class needs documentation (most of the module documentation
 should be moved here)

 I've added documentation but it might need to be added more.
 {{{
 4b368b5 [trac2225_statistics] correct and add documentation of the
 Counters class
 }}}

 > * `Counter.__init__()` should be documented to indicate what happens
 when `spec_file_name` is passed, and in the case where it is `None`.

 I've documented about the cases that the spec file is specified and
 omitted.
 {{{
 dbf92ea [trac2225_statistics] document about cases when a spec file is
 specified and omitted
 }}}

 > * What is the purpose of `Counter._rlock`? If it is necessary, shouldn't
 access to `Counter._disabled` also be synchronized by using it, in
 `.enable()`, `.disable()`, `.inc()`, `.dec()`, etc.?

 It is necessary because counters can be incremented by the different
 threads.  So it should as you mentioned. Also a timer cannot be update if
 it's disabled.
 {{{
 2f7080c [trac2225_statistics] synchronize when accessing self._disabled
 }}}

 > * The following repeated code is best moved to a function, so that we
 have a single place to update if we change how the key is computed from
 the args.
 > {{{
 >       identifier = '/'.join(args)
 > }}}

 I've added a helper method _concat() and replaced with it.
 {{{
 301ccaf [trac2225_statistics] add a helper method _concat and its test
 }}}

 > * If `Counter.inc()` and `.dec()` are largely similar, except for the
 `step`, they should be unified by moving most of the code to a common
 function.

 I've added step in kwarg in inc() and directly used it in dec().
 {{{
 4ae14bd [trac2225_statistics] add step as kwarg into inc() and use it in
 dec() with step=-1
 }}}

 > * In `_add_counter()`, is the `try` necessary?
 > {{{
 >     try:
 >         isc.config.find_spec_part(spec, identifier)
 >     except isc.cc.data.DataNotFoundError:
 >         # spec or identifier is wrong
 >         raise
 > }}}

 No, it was not neccesary. Thanks.
 {{{
 7ec3307 [trac2225_statistics] remove an unnecessary try/expect statement
 }}}

 > * Is there a test for the reason why the format string is used here:
 > {{{
 >     spec_ = isc.config.find_spec_part(
 >       spec, '%s' % identifier.split('/')[0])
 > }}}

 I've removed the format string, thanks.
 {{{
 0d0de6a [trac2225_statistics] remove an unnecessary formating
 }}}

 > * The behavior of `_set_counter()`, `_get_counter()` and
 `_inc_counter()` seem inconsistent when `identifier` isn't already known.
 `_set_counter()` and `_inc_counter()` both add it if necessary, but
 `_get_counter()` doesn't (and throws).


 Sorry, I couldn't understand why `_get_counter()` should add. I think
 `_get_counter()` should just return the current value and should not
 change the statistics data. OTOH `_set_counter()` and `_inc_counter()` are
 intended for adding an entry into statistics data if there is not.

 `_get_counter()` is not indended to be used before the counter is
 incremented. It's mostly for a testing purpose.

 > * If the timer sets the delta time in seconds, isn't it simpler to use
 `time.time()` instead of `datetime` and `timedelta`? It would also avoid
 the `timedelta` to seconds computation in `_stop_timer()`.

 I decided to use datetime.now() because of the following python reference:
 http://docs.python.org/3.2/library/datetime.html#datetime.datetime.now
 {{{
 supplies more precision than can be gotten from going through a
 time.time() timestamp
 }}}
 But we can use timedelta.total_seconds() instead of such seconds
 computation. I've revised the code.

 {{{
 868558c [trac2225_statistics] use timedelta.total_seconds() instead of
 redundant computation
 }}}

 > * `Counter.stop()` documentation should say what units of time (seconds,
 microseconds, etc.)

 I've updated documentaion.
 {{{
 1f05684 [trac2225_statistics] update document of Counters.stop()
 }}}

 > * In `Counter.stop()`, I suggest changing it so it returns upon
 `isc.cc.data.DataNotFoundError` instead of using the `else`, but it's just
 a matter of personal style I guess. Please consider it and decide what you
 prefer best.

 I've removed 'else', thanks.
 {{{
 289118e [trac2225_statistics] remove 'else' statement
 }}}

 > * I suggest renaming `Counter.start()` to indicate that it is a timer
 that is started, i.e., `Counter.start_timer()`.

 I've renamed start()/stop(), thanks.
 {{{
 db1ad53 [trac2225_statistics] rename start()/stop() to
 start_timer()/stop_timer()
 }}}

 > * I also suggest renaming `Counter.clear_counters()` to
 `Counter.clear()` or `.clear_all()` (see also the suggestion above about
 renaming the class to `Counters`). I don't know if `Counter` is intended
 to have any singleton behavior, but if it does, its doc should also be
 updated to indicate that.

 I've renamed it to `clear_all()`, thanks.  But I left documentation about
 the above discussion on singleton.

 {{{
 bfb0225 [trac2225_statistics] rename to clear_all()
 }}}

 > * The expression below should be commented (I see what it does, but a
 comment would help a reader quickly understand). You may be able to get
 this from the `args` argument itself rather than splitting `identifier`.
 Also as I mentioned above, it's best to do such key computation in a
 separate method so it can be understood and updated easily.
 > {{{
 >               '/'.join(identifier.split('/')[0:-1]))\
 >                 [identifier.split('/')[-1]]
 > }}}

 I've added documents. Is it more helpful?
 {{{
 2aa30dc [trac2225_statistics] add helpful comments to the complicated
 expression
 }}}

 > * In `Counter.dump_statistics()`, is the comment always correct?
 > {{{
 >       # If self.statistics_data contains nothing of zone name, it
 >       # returns an empty dict.
 >       if self._perzone_prefix not in statistics_data:
 >             return statistics_data
 > }}}

 It's not correct. I've updated.
 {{{
 4688552 [trac2225_statistics] update document as it is
 }}}

 > * Is `get_statistics()` a better name than `dump_statistics()` (as it
 returns a dict)?

 I've renamed.
 {{{
 4661800 [trac2225_statistics] rename dump_statistics() to get_statistics()
 }}}

 > Locally I also updated the module documentation to make it more clear,
 but now I think it's best we address the comments above first. I'll update
 the resulting docs after that which you can review.

 Thank you very much! I've also updated documentation above but it might
 not be enough. Please review again.

 Regards,

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