BIND 10 #930: update stats modules to get statistics list from cfgmgr, and define the list in spec file of each module
BIND 10 Development
do-not-reply at isc.org
Tue Aug 2 13:35:14 UTC 2011
#930: update stats modules to get statistics list from cfgmgr, and define the
list in spec file of each module
-------------------------------------+-------------------------------------
Reporter: | Owner: vorner
naokikambe | Status: reviewing
Type: | Milestone:
enhancement | Sprint-20110802
Priority: major | Resolution:
Component: | Sensitive: 0
statistics | Sub-Project: DNS
Keywords: | Estimated Difficulty: 20.0
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by naokikambe):
* owner: naokikambe => vorner
Comment:
Replying to [comment:8 vorner]:
Thank you for reviewing.
I added more changes into trac930 branch after you reviewed codes first.
Because we must add more changes depending on #928 and #929 as these
reviewing was going on. Maybe you have already read until git
5909fb3d1dd3cd806e0df965d707b45803d4b9c3. Could you see after git
a0a022bf4c5150f62b8a4118e5d6a4dcaf69f669? Thank you very much for
patience.
> * The changelog could be more user oriented. At talks a lot about how
data are sent trough the system, which users aren't interested much. I
think the main point is that the items are specified by each module's spec
file instead of one global.
I made it easier. How about this?
{{{
xxx. [func] naokikambe
Statistics items are specified by each module's spec file.
Stats module can read these through the config manager. Stats
module and stats httpd report statistics data and statistics
schema by each module via both bindctl and HTTP/XML.
(Trac #928,#929,#930, git
nnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn)
}}}
> * The name of `parse_spec` doesn't look like the best choice, because
the spec file is parsed already, this function extracts the default values
of items. So, something like `get_spec_defaults` would look better.
I renamed the function name at git
24fa3987d6e1f42d645f2c0a9c26cd8bdbc9c16b.
> * If there are errors in `command_show`, an exception is raised, but
the errors themselfs are not preserved anywhere. It would be better to
give as much information as possible. Would it be possible to either log
the errors or put them into the exception?
I fixed that at git 065c6004799a76d45859289d2ebae0379e9dceba.
> * A lot of tests use time.sleep. I think this might be a problem, as
our buildbots showed, the timers there are highly inaccurate and they are
sometimes loaded. It might happen the timeout will not be enough. Or, if
it would be large enough to run just about everywhere, it would be
incredibly slow to run the tests. As this looks like it is some kind of
synchronization between threads. I think it would be better to use some
tool that is designed for this (some condition or event or whatever it is
called in python).
I could not find nicer solution:(
Could you give some example or can it be out of scope on this ticket?
> * This is not a problem, but I wonder, where these come from? I know
the test does invoke some 500s, but who prints the messages?
> {{{
> localhost - - [14/Jul/2011 12:17:58] code 500, message Internal Server
Error
> localhost - - [14/Jul/2011 12:17:58] "GET /bind10/statistics/xml
HTTP/1.1" 500 -
> localhost - - [14/Jul/2011 12:17:58] code 500, message Internal Server
Error
> localhost - - [14/Jul/2011 12:17:58] "GET /bind10/statistics/xsd
HTTP/1.1" 500 -
> localhost - - [14/Jul/2011 12:17:58] code 500, message Internal Server
Error
> localhost - - [14/Jul/2011 12:17:58] "GET /bind10/statistics/xsl
HTTP/1.1" 500 -
> }}}
Actually I didn't implement it. I think http.server.log_message might
print theses messages.
We need to add more messages helpful for users?
> * There are many places where there's an empty line containing only
spaces. This is not a huge problem, but the code looks better if there are
no spaces at the end of line or on empty lines. The file having most of
them is `src/bin/stats/tests/b10-stats-httpd_test.py`, but some are in
others as well. Git can show diff with them marked, if you set color.diff
= auto (I believe it is this one, maybe it's a different color config).
I might mistake somewhere when I copied some codes. I removed the
unnecessary tailing whitespaces.
Please see git f02cc6985eff417f34e4edd232b6836852de6c98.
> * This test looks really strange:
> {{{#!python
> def test_get_timestamp(self):
> self.assertEqual(stats.get_timestamp(), 1308730448.965706)
> }}}
> I think it does depend on being run after another test that sets the
function to get the current time to constant one. But depending on order
of tests is fragile, the setup should be put into setUp() or somewhere
like that and this test would deserve a comment about it. There are more
tests like this.
> * This is unneeded, isn't it?
> {{{#!python
> tearDown():
> pass
> }}}
I fixed at git 5002ef4bc64e1d58293d8891adbaf652e075a6cf. Please see that.
> * We run the tests from source? Well, we do, but I mean with the
environment variable? Where does this come from?
> {{{#!python
> self.assertTrue("B10_FROM_SOURCE" in os.environ)
> }}}
We set os.environ!["B10_FROM_SOURCE"] when doing "make check" as the
following. We are sure that is set properly in the test. However actually
this test is not so important. We need to remove this?
tests/Makefile.am:
{{{
for pytest in $(PYTESTS) ; do \
...
B10_FROM_SOURCE=$(abs_top_srcdir) \
...
$(PYCOVERAGE_RUN) $(abs_srcdir)/$$pytest || exit ; \
done
}}}
> * Generally, the tests where the whole system is simulated is hard to
call a "unit test“. I'm not opposed to having them, having more and more
thorough tests is good for sure, but some kind of comment on what is going
on there would be nice.
I added comments into test scripts at git
fee53aeaaf8c953c856f12ae3388a61abc47d2f9, but I could not remeber nicer
comments. Could you give me advice?
> * A lot of functions (eg. update_modules) would deserve a little bit
more documentation. For example, some of them return some kind of None in
case of bad name and are handled by a condition - this special return
value should be noted (or, better, this should be thrown as some kind of
exception and caught at the level where the command is received from CC,
automatically converting to the error answer - but it even the exception
should be noted in the docstring).
I modified the behaviors of these methods. Please see git
5c9bdf1345699e47018c16254778de67cc695847.
Thank you.
--
Ticket URL: <http://bind10.isc.org/ticket/930#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list