Opened 9 years ago

Closed 8 years ago

#928 closed enhancement (fixed)

Add statistics category into spec file of each module, and update cfgmgr to handle it

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: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Each module instead of stats module should have statistics items in its own spec file as result of the discussion of 3:ticket:719. So add a new category "statistics" in spec file of each module, and update cfgmgr to handle it. A proposed data structure of the spec file is:

{ "module_spec": {
    "module_name": "my_own_module",
    "module_description": "foo",
    "config_data": [ <list of configurable options> ],
    "commands": [ <list of commands> ],
    "statistics": [ <list of collected statistics> ]
  }
}

Subtickets

Change History (10)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 8 years ago by naokikambe

  • Owner set to UnAssigned
  • Status changed from new to reviewing

It's ready for reviewing.

comment:3 Changed 8 years ago by naokikambe

  • Estimated Difficulty changed from 0.0 to 5.0

comment:4 Changed 8 years ago by stephen

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

comment:5 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to naokikambe

I think it would be wise to also add some code to check the specification of the statistics itself; see the function check_data_specification() in module_spec.cc (which starts the checks for config items and commands). Looks like right now the actual specs for statistics are the same as those for config items so a new function check_statistics_item_list could probably be a copy of that (or it could just call that function if they are exactly the same). I'm worried that if we make a mistake in the specs for statistics it'll fail at a seemingly random point later.

Are statistics mandatory in a spec? (i see there are a lot places where statistics is defined in the specfile for a module but it's just an empty list) Not that I have a big problem with that but if they are optional we probably don't need that.

meta-question: what's the final commit there? a merge of the branch itself? (3225575a1d7dc447b802efe5cc15687465acbbc4)

comment:7 in reply to: ↑ 6 Changed 8 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Thank you for reviewing.

Replying to jelte:

I think it would be wise to also add some code to check the specification of the statistics itself; see the function check_data_specification() in module_spec.cc (which starts the checks for config items and commands). Looks like right now the actual specs for statistics are the same as those for config items so a new function check_statistics_item_list could probably be a copy of that (or it could just call that function if they are exactly the same). I'm worried that if we make a mistake in the specs for statistics it'll fail at a seemingly random point later.

That's right. I added check_statistics_item_list and the related methods into module_spec.cc.
Please see git e454c03a1226fbc90d7f58229a91f3e4bc3775c3 in trac929 (not in trac928, Sorry for confusion.)

Are statistics mandatory in a spec? (i see there are a lot places where statistics is defined in the specfile for a module but it's just an empty list) Not that I have a big problem with that but if they are optional we probably don't need that.

Not mandatory. I removed empty statistics categories. Please see git 7018b05373f706d54a6a0fc2d8977c44292b273f in trac928.

meta-question: what's the final commit there? a merge of the branch itself? (3225575a1d7dc447b802efe5cc15687465acbbc4)

I mistook pushing. I repushed in forcely. Please repull from the remote branches.

Sorry for inconvenience.

comment:8 Changed 8 years ago by jelte

  • Owner changed from jelte to naokikambe

ah ok, in that case this looks ok. Do you want to merge it one at a time or simply merge trac929 (or 930) in one go and drop this branch?

comment:9 Changed 8 years ago by naokikambe

Thank you for reviewing. Source codes in #928 are included in both #929 and #930. I will not merge #928 only or #929 only. After both #929 and #930 will be fixed, I will merge #930 only. But I will close this ticket and will drop the branch if both #929 and #930 are fixed. I leave #928 opened until that.

comment:10 Changed 8 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.

Note: See TracTickets for help on using tickets.