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

[BIND 10 Development]

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

 * owner:  muks => naokikambe


Comment:

 Hi Kambe san

 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.

 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.

 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.

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

 * 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()`?

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

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

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

 * 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)
 }}}

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

 * 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
 }}}

 * 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])
 }}}

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

 * 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()`.

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

 * 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 suggest renaming `Counter.start()` to indicate that it is a timer that
 is started, i.e., `Counter.start_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.

 * 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]]
 }}}

 * 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
 }}}

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

 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.

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