BIND 10 #2136: update the stats daemon according to the new statistics model

[BIND 10 Development]

#2136: update the stats daemon according to the new statistics model
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  naokikambe
  naokikambe                         |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120821
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  statistics                         |  Estimated Difficulty:  8? (depends
                   Keywords:         |  on #2135)
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => naokikambe


Comment:

 Hi Kambe-san

 Here are my comments.

 * It's better to put `src/bin/stats/tests/b10-config_test.db` in
   `src/bin/stats/tests/testdata/` sub-directory as we follow that
   convention for all test related data files. You may want to consider
   naming it just `b10-config.db` too.

 * This config file (`b10-config_test.db`) should be multi-line and
   indented so that everything is not in a single line. Any patching will
   modify the entire file and it will take time to check what changed.

 * The branch preserves all statistics data including the data for
   processes which no longer exist. What was the reason for this change?

 * What do we do about this FIXME in `stats.py.in`:

 {{{
                 # FIXME: A issue might happen when consolidating
                 # statistics of the multiple instances. If they have
                 # different statistics data which are not for adding
                 # each other, this might happen: If these are integer
                 # or float, these are added each other, otherwise
                 # these are overwritten into one of them.
 }}}

 * I have pushed some changes to the branch, where discussion was not
   necessary. Please check these commits on the branch.

 * Is the following code what you intended to write (in tests)? `t`
   is unused and the loop checks the same assertion many times.

 {{{
             for s in stat.statistics_data_bymid['Auth'].values():
                 for t in s.values():
                     self.assertEqual(
                         s, {'queries.perzone': auth.queries_per_zone,
                             'queries.tcp': auth.queries_tcp,
                             'queries.udp': auth.queries_udp})
 }}}

 * Will the show command now force a polling if (current_time-
 lasttime_poll) < 1) ?
   Why not simply show what data is available and let the poll-interval
   be used for updating?

 * I am not sure if using message bus address is a correct way of finding
   multiple instances of the same module. In any case, the new code in
   the following (`do_polling()`) uses int indexes and types, instead of
   looking for keys. Is it possible to rewrite it to look for particular
   keys? Are such keys available?

 {{{
 -            if rcode == 0 and 'components' in value:
 -                modules = [ c['special'].capitalize() \
 -                                for c in value['components'].values() \
 -                                if 'special' in c ]
 +            if rcode == 0 and type(value) is list:
 +                modules = [ v[2] if type(v) is list and len(v) > 2 \
 +                                else None for v in value ]
 }}}


 * In some commits, unrelated changes are mixed. For example, in commit
   `1c195bb85881b91fae165ef5b8334da8b6611a00`, the set command is
   removed, but `poll-interval` is also documented. Such mixed commits
   are difficult to follow when reviewing. In this case, the
   `poll-interval` manpage change should go into the commit introducing
   that config setting, but in case we forgot it, it should become a
   separate commit just adding the manpage change. Another example is
   commit `4a50ef0287747928c8dbd4d045df9780df3c3840` where the changes to
   `test_utils.py` do not belong in that commit.

 I have reviewed the commits one-by-one, and that the lettuce and system
 tests pass. Please assign it back to me when you are done with the above
 changes and I'll re-review the code change as a whole once.

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