Opened 9 years ago

Closed 9 years ago

#929 closed enhancement (fixed)

Add an interface into cfgmgr to get statistics list of each module

Reported by: naokikambe Owned by: naokikambe
Priority: medium Milestone: Sprint-20110816
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 15.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Current stats module has each module's statistics list in its own spec file. However, after #928, stats module should get the list from cfgmgr. So add an interface into cfgmgr so that stats modules get statistic list of each module. For example, a command "get_statistics_spec" may be added into cfgmgr for this.

Subtickets

Change History (21)

comment:1 Changed 9 years ago by vorner

This, in fact, might be unnecessary. The config manager has a command 'get_module_spec'. That one sends the whole spec, so including any possible statistics definitions. I guess that the spec parsers will have a function added, so besides of getModuleConfig and getModuleCommands, there'll be getModuleStats, which might then be used to easily get the statistics.

comment:2 Changed 9 years ago by naokikambe

  • Type changed from defect to enhancement

comment:3 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:4 Changed 9 years ago by naokikambe

  • Estimated Difficulty changed from 0.0 to 15.0
  • Owner set to UnAssigned
  • Status changed from new to reviewing

It's ready for reviewing. I had implemented get_statistics_spec method. Please see the branch for detail.

comment:5 Changed 9 years ago by stephen

  • Milestone changed from Year 3 Task Backlog to Sprint-20110802

comment:6 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:7 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

hey I noticed the commit log in the branch seems somewhat off, i see a lot of commits twice (with some commits from other branches in between). Can it be that you rebased a branch that was already pushed upstream or something? (if you are available somewhere in the near future perhaps we can talk about this on jabber, i think this may also be the case in the two other branches)

In general, the code looks ok, tests are nice, and additions seem to work nicely with the existing code.

One question; Won't we need a validateSpec (and getStatisticsSpec or something) in the c++ variant? (perhaps not, but in that case we might want to make a note of that somewhere)

and just a trivial comment in module_spec.py:

in validate_statistics():

if stat_spec != None:

this should work correctly (in general), but it is slightly better to use

if stat_spec is not None:

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Thank you for reviewing.

Replying to jelte:

hey I noticed the commit log in the branch seems somewhat off, i see a lot of commits twice (with some commits from other branches in between). Can it be that you rebased a branch that was already pushed upstream or something? (if you are available somewhere in the near future perhaps we can talk about this on jabber, i think this may also be the case in the two other branches)

As I mentioned in trac928, I mistook pushing. I corrected it. Please repull from the remote repository.

In general, the code looks ok, tests are nice, and additions seem to work nicely with the existing code.

One question; Won't we need a validateSpec (and getStatisticsSpec or something) in the c++ variant? (perhaps not, but in that case we might want to make a note of that somewhere)

I added c++ valiant. I pushed git e454c03a1226fbc90d7f58229a91f3e4bc3775c3 in trac929. Please see it.

and just a trivial comment in module_spec.py:

in validate_statistics():

if stat_spec != None:

this should work correctly (in general), but it is slightly better to use

if stat_spec is not None:

I fixed in the former way as you pointed out. I pushed git 211f6c58e8cd6311eaca0a2ba181c4bfb379476c in tac929. Please see it.

Thanks,

comment:9 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

Replying to naokikambe:

in validate_statistics():

if stat_spec != None:

this should work correctly (in general), but it is slightly better to use

if stat_spec is not None:

I fixed in the former way as you pointed out. I pushed git 211f6c58e8cd6311eaca0a2ba181c4bfb379476c in tac929. Please see it.

Actually, this is indeed better than just 'if stat_spec', but what i really meant was that 'is not None' is preferrable over '!= None' (more efficient and cleaner). I realize that I had myself not used this, so to make up for it I replaced all 12 occurrences (in your branch), please see commit e54237acf184e5dc6d03409869b525152a259f2a.

However, I now get a new problem, when I start bind10 locally, the statistics module keeps exiting with a stacktrace due to a bad statistics specification (for the timestamp with format 'second'). Do you see this as well?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Replying to jelte:

Actually, this is indeed better than just 'if stat_spec', but what i really meant was that 'is not None' is preferrable over '!= None' (more efficient and cleaner). I realize that I had myself not used this, so to make up for it I replaced all 12 occurrences (in your branch), please see commit e54237acf184e5dc6d03409869b525152a259f2a.

Thank you for fixing my inappropriate way and sorry for my misunderstanding. The all changes are good.

However, I now get a new problem, when I start bind10 locally, the statistics module keeps exiting with a stacktrace due to a bad statistics specification (for the timestamp with format 'second'). Do you see this as well?

Thank you for notice. I couldn't find there was that runtime error on stats and stats_httpd. The reason of that error was that stats-schema.spec had the invalid format_type in the timestamp item. I fixed that and also modified mock ModuleSpec under the stats/tests/isc/config. But Those will be removed in #930. Please see git 847df8f9c0ccd43f296e79df6ecd92d77e3da32a and c290f8bb897e45d304b2895c394a421f46a2ee68, and please check whether there is no more runtime error on stats and stats_httpd if possible. Of course, I've already checked both.

Best

comment:11 in reply to: ↑ 10 Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

Replying to naokikambe:

Replying to jelte:
Thank you for fixing my inappropriate way and sorry for my misunderstanding. The all changes are good.

well it was my explanation and initial implementation that was wrong :)

However, I now get a new problem, when I start bind10 locally, the statistics module keeps exiting with a stacktrace due to a bad statistics specification (for the timestamp with format 'second'). Do you see this as well?

Thank you for notice. I couldn't find there was that runtime error on stats and stats_httpd. The reason of that error was that stats-schema.spec had the invalid format_type in the timestamp item. I fixed that and also modified mock ModuleSpec under the stats/tests/isc/config. But Those will be removed in #930. Please see git 847df8f9c0ccd43f296e79df6ecd92d77e3da32a and c290f8bb897e45d304b2895c394a421f46a2ee68, and please check whether there is no more runtime error on stats and stats_httpd if possible. Of course, I've already checked both.

Indeed, it runs for me now, and as that addition in the mock part will be removed, so this ticket can also be merged (as part of #930) and closed.

comment:12 Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to jelte

I found a trivial bug in the check function for date and time string(check_format). I fixed it at git e9620e0d9dd3d967bcfb99562f13848c70538a44. In the revised code it checks reversibly. I pushed some changes to both trac929 and trac930. Could you see code more?

comment:13 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

Looks OK, but two comments;

  • there doesn't appear to be a test for that change (if i remove the added check, it doesn't appear to fail), is it possible to add one?
  • fixed-size buffers are always scary wrt buffer overflows. From the looks of the code, it seems to be safe here, but please add a comment why this is so (also so that if anyone wants to change said code he is aware of any danger)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Replying to jelte:
Thank you for giving more comments.

  • there doesn't appear to be a test for that change (if i remove the added check, it doesn't appear to fail), is it possible to add one?

I added such tests. It's git 330dd7915a9a6b808db7b65b236e733e42401b8d.

  • fixed-size buffers are always scary wrt buffer overflows. From the looks of the code, it seems to be safe here, but please add a comment why this is so (also so that if anyone wants to change said code he is aware of any danger)

I couldn't consider enough for buffer overflows. I changed more the code to check time format. It's git-id same to above. Sorry, could you see that?

Thanks,

comment:15 in reply to: ↑ 14 Changed 9 years ago by naokikambe

Hello Jelte-san,

Sorry, the changes are OK for you? This changes are already merged into trac930, and trac930 is almost ready for merging into master. If this changes are ok, I will merge trac930 into master. Could you let me know?

Best

comment:16 Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

Yes, sorry, those changes look OK

comment:17 Changed 9 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed

I've just merged with master, and I'm closing the ticket. Thank you for reviewing.

comment:18 Changed 9 years ago by naokikambe

  • Resolution fixed deleted
  • Status changed from closed to reopened

Jelte-san,

Sorry, I failed to merge trac930 because some failures was occurred in buidbot after merging. But trac929 (and trac928) is OK and only trac930 is problem, so I'd like to merge only it into master. I'd like not to merge trac930 for a while. I will fix the problem of trac930 on another ticket(maybe #1175). Is that OK? If OK, I'll merge trac929 (and trac928).

Thanks,

comment:19 Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to jelte
  • Status changed from reopened to reviewing

comment:20 Changed 9 years ago by jelte

  • Owner changed from jelte to naokikambe

sure, no problem, but get it out of my ticket queue ;)

comment:21 Changed 9 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed

I finished merging. (Actually, I've reverted trac930 codes discarded yesterday.)
Thanks,

Note: See TracTickets for help on using tickets.