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:  muks
  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 naokikambe):

 * owner:  naokikambe => muks


Comment:

 Hello Mukund-san,

 Thank you for reviewing!

 Replying to [comment:17 muks]:
 > * 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.

 I moved it under stats/tests/testdata and rename it to b10-config.db.
 Please refer to:
 {{{
 e0a964e [2136] Changes regarding the test config file
 }}}

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

 I described so in StatsModule when working on #2135. But originally I
 intended that it is for fixing the defect #1941(stats lossage (multiple
 auth servers)). For discarding statistics data of an inactive instance,
 PID is required. Because PID is used for identifying whether such process
 is running or dead. A list of PIDs is obtained from the result of the Boss
 command ''show_processes''.
 At this branch, PID isn't used for identifying instances, so the stats
 module cannot discard statistics data of an inactive instance and doesn't
 need to do so. IMO the stats module might not have to consolidate
 statistics data by each instance if we also consider the below issue.

 > * 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 cannot find a reasonable solution for it.:-( Only summable statistics
 type, e.g integer or real, should be defined in the statistics spec of a
 module which might run multiply. I added a minor workaround for this: If
 two values are string type, they are consolidated into bigger one. If one
 of them is ''None'' type, they are consolidated into another one.
 Regarding this workaround, please refer to:
 {{{
 003d027 [2136] revised the internal method _accum()
 }}}

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

 These make sense for me. Thank you very much!

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

 That's right! I revised it at:
 {{{
 ec6716e [2136] removed an unnecessary loop
 }}}

 >
 > * 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 at first imagined that a command like "force-poll" might be required.
 But I got the following Jinmei-san's comment when I worked on #2135. I
 finally agreed and followed it.
 https://lists.isc.org/pipermail/bind10-dev/2012-July/003698.html
 {{{
 What I envisioned is a revision of the "show" command; the new version
 basically immediately invokes the "send statistics" request from stats
 to the module(s).  We probably want to rate-limit this, e.g., at most
 1 request per second though.  The stats daemon wait for the
 response(s), and sends the result back to cmdctl.
 }}}

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

 Neither am I. I think it might be possible. But I am thinking that this is
 just a workaround for releasing the statistics feature by the end of
 Sept.. IMO this part should be revised in the near future. I added a note
 related to above at:
 {{{
 2fc9bd6 [2136] added a note about a value from the Boss command
 show_processes
 }}}

 > * 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've revised these incorrect messages, so the git ids are changed to:
 {{{
 94d9320 [2136] removed 'set' command and introduced a 'poll-interval'
 config item
 9d21102 [2136] added some misc changes
 }}}
 Old commits were changed. Could you recheckout and review again?

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

 I give you. Thank you for continuing to review.

 BTW I'm sorry. I added more changes not related to above:
 {{{
 07a1ba0 [2136] moved invoking update_modules() to outside of the method
 4b19f8e [2136] corrected the improper test
 37ce3a0 [2136] removed the do_polling method from the __init__ method of
 stats.Stats()
 }}}
 I think these issues should be fixed in this ticket. Could you also review
 them?

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