BIND 10 #2884: per zone statistics must be separated per class

[BIND 10 Development]

#2884: per zone statistics must be separated per class
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  naokikambe
            Priority:  medium        |                       Status:
           Component:  statistics    |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130709
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  8             |                 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 :)

 Here is my review.

 * In `item_title` and `item_description`, I suggest you use "RR class
   name" as it's more indicative. Otherwise people may assume wrong kind
   of class. Or you can use the text in the manpage ("Class name of the
   zone, e.g. IN, CH, and HS").

 * Is there any validation done for the RR class, i.e., that it must be
   in ['IN', 'CH', etc.]?

 * Please rewrite the following code in `get_statistics()` in
 `counters.py`:
 {{{
                     id_str = '%s/%%s/%s' % (cls, attr)
                     id_str1 = id_str % zone
                     id_str2 = id_str % self._entire_server
 }}}

 to something like:

 {{{
                     id_str1 = '%s/%s/%s' % (cls, zone, attr)
                     id_str2 = '%s/%s/%s' % (cls, self._entire_server,
 attr)
 }}}

 As a coding style, we should be very careful not to construct format
 strings from other input. They could include formatting of their own.

 In a language like C, it's usually bad practise to use non-static format
 strings; we can sometimes construct it with lengths and such, but it's
 very bad to copy input string data into it. I doubt this can be a
 security issue in Python, but it might lead to bugs if a % is somehow
 introduced. The updated code above is easier to follow too.

 * In `xfrout_test.py.in`, it may be worth adding a `TEST_RRCLASS_STR` at
   the top to avoid the many `to_text()` calls.

 * I suggest some more code comment at the top of `_add_counter()` (maybe
   with an example identifier) so it is easily readable, esp. for the
   code following the statement:
 {{{
     if has_named_set:
 }}}

   I follow what the code is doing now, but it took me 5 minutes for
   running the code in my head. :)

 * The rest of the branch looks ok.

 * The `ChangeLog` entry looks ok. `make check` and lettuce pass on
   it. There are no `Makefile.am` changes, so I have not run
   `make distcheck` on it.

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