Opened 9 years ago

Closed 9 years ago

#763 closed enhancement (complete)

Conversion of statistics code to use the new logging interface

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

Description

This is dependent on ticket #756, and involves converting the files related to the statistics code to use the new logging interface.

Subtickets

Change History (11)

comment:1 Changed 9 years ago by stephen

  • Defect Severity set to N/A
  • Milestone changed from Year 3 Task Backlog to Sprint-20110628
  • Sub-Project set to DNS

comment:2 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 2

comment:3 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to accepted

comment:4 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from accepted to reviewing

ready for review. In order for the tests to work, i had to copy python/isc/log/init into bin/stats/tests/isc (which contains a big skeleton framework of a large part of the python library, something we should remove IMO, but certainly out of scope for this ticket).

stats contains 2 modules, b10-stats itself and b10-stats-httpd. Creates .mes for both, converted logging, and updated handling of the -v switch to set severity to DEBUG and debuglevel to 99.

comment:5 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 Changed 9 years ago by naokikambe

Jelte-san, thank you for coding. I don't see the pushed codes yet, but I'll remove almost these ugly skeletons in #930. I'll push ASAP in #930.

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

  • Owner changed from stephen to jelte

src/bin/stats/stats_httpd.py.in
Nothing to do with this ticket, but I note that some files are stats_httpd and others are stats-httpd. I would suggest that they be made consistent.

src/bin/stats/stats_httpd_messages.mes
With the convention that messages are <prefix>_<message>, I was reading a message such as STATS_HTTPD_CLOSING_CC_SESSION as prefix STATS_, message HTTPD_CLOSING_CC_SESSION. I suggest we use a string (without underscros) for the prefix. In this case, how about something like STHTTPD_ or HTTPDSTAT_?

STATS_HTTPD_BAD_OPTION_VALUE
Should be no comma before "and".

STATS_HTTPD_CC_SESSION_ERROR
I've seem traffic in the Jabber room on this. Are we standardising on "message bus" or "message queue"?

STATS_HTTPD_CLOSING_CC_SESSION
"stats httpd" should be hyphenated to be consistent with other uses.

STATS_HTTPD_CLOSING
Typo "te".

STATS_HTTPD_SERVER_ERROR
"This is an error condition that should not occur". Would it be an error if it should? :-)

STATS_HTTPD_SERVER_INIT_ERROR
Receiving new configuration data". This implies that it has old configuration data which is being updated, and so suggests that has been running some time and so is not initializing.

Should be space in "data.The".

STATS_HTTPD_SHUTDOWN
"will now shut down" implies that the daemon is just about to start shutting down, while the message "shutting down" implies that it is in the process of shutting down.

STATS_HTTPD_STOPPED_BY_KEYBOARD
Perhaps add "(Obviously, this can only happen if the statistics daemon is being run from a terminal.)"

STATS_HTTPD_UNKNOWN_CONFIG_ITEM
Received a new configuration from where, and where was the response sent?

src/bin/stats/stats_messages.mes
In message descriptions, I think "stats" should be expanded to "statistics".

STATS_BAD_OPTION_VALUE
Should be no comma before "and".

STATS_CC_SESSION_ERROR
Same comments as STATS_HTTPD_CC_SESSION_ERROR

STATS_RECEIVED_NEW_CONFIG
In light of the name, the text might be better as "the statistics module has received a new configuration from the configuration manager" rather that "configuration manager has sent ...". The latter seems to imply that this should be a message output by the configuration manager.

STATS_RECEIVED_REMOVE_COMMAND
Can we explain what a "remove" command is and what is the "given name"? Also, does this have a permanent effect (i.e. when I run it, I can never add given name back)?

STATS_RECEIVED_RESET_COMMAND
"reset all collected statistics": to what? Would be better to say that the reset command clears the stored statistics?

STATS_RECEIVED_SHUTDOWN_COMMAND
Should be no comma before "and".

STATS_RECEIVED_STATUS_COMMAND
I feel that "I'm alive" is a bit flippant. Assuming that the only status that can be returned is "I'm alive", perhaps something like "The statistics module has received a command asking it to report its status. It will return a response indicating that it is running normally."

STATS_RECEIVED_UNKNOWN_COMMENT
Should be no comma before "and".

STATS_SEND_STATS_REQUEST_BOSS
If this is the only request that can be sent to Boss, perhaps just "STATS_SEND_REQUEST_BOSS" would be enough?

Being pedantic here, The word "statistics" refers to a quantity (such as the mean or median) calculated from a set of data. Is the statistics module requesting Boss to send it this information, or is it requesting Boss to send it the raw counters from which it will calculate appropriate statistics?

STATS_STOPPED_BY_KEYBOARD
Perhaps add "(Obviously, this can only happen if the statistics daemon is being run from a terminal.)"

comment:8 in reply to: ↑ 7 Changed 9 years ago by jelte

Replying to stephen:

src/bin/stats/stats_httpd.py.in
Nothing to do with this ticket, but I note that some files are stats_httpd and others are stats-httpd. I would suggest that they be made consistent.

I agree, but I don't want to do it in this ticket. It's a minor change to sync them but I think we should sync that with jprs in case they are working on it.

src/bin/stats/stats_httpd_messages.mes
With the convention that messages are <prefix>_<message>, I was reading a message such as STATS_HTTPD_CLOSING_CC_SESSION as prefix STATS_, message HTTPD_CLOSING_CC_SESSION. I suggest we use a string (without underscros) for the prefix. In this case, how about something like STHTTPD_ or HTTPDSTAT_?

made it STATHTTPD_

STATS_HTTPD_BAD_OPTION_VALUE
Should be no comma before "and".

ok

STATS_HTTPD_CC_SESSION_ERROR
I've seem traffic in the Jabber room on this. Are we standardising on "message bus" or "message queue"?

i don't know. Made it 'bus'.

STATS_HTTPD_CLOSING_CC_SESSION
"stats httpd" should be hyphenated to be consistent with other uses.

done

STATS_HTTPD_CLOSING
Typo "te".

ack

STATS_HTTPD_SERVER_ERROR
"This is an error condition that should not occur". Would it be an error if it should? :-)

sorry, jinmei has been hammering on adding such lines :p removed it

STATS_HTTPD_SERVER_INIT_ERROR
Receiving new configuration data". This implies that it has old configuration data which is being updated, and so suggests that has been running some time and so is not initializing.

s/new/its/

Should be space in "data.The".

added

STATS_HTTPD_SHUTDOWN
"will now shut down" implies that the daemon is just about to start shutting down, while the message "shutting down" implies that it is in the process of shutting down.

changed to is shutting down

STATS_HTTPD_STOPPED_BY_KEYBOARD
Perhaps add "(Obviously, this can only happen if the statistics daemon is being run from a terminal.)"

nah, seems too obvious :p i've just copy-pasted this one from all the other modules where it occurs, so this message is more consistent :)

STATS_HTTPD_UNKNOWN_CONFIG_ITEM
Received a new configuration from where, and where was the response sent?

added 'from the configuration manager'

src/bin/stats/stats_messages.mes
In message descriptions, I think "stats" should be expanded to "statistics".

Hmm, I disagree, actually. Not too strongly, but I've been using 'stats module' to point to the module (the name of the module being 'stats'), and 'statistics' to point to the data. If you do have a strong opinion that it should also be 'statistics module' we can change it.

STATS_BAD_OPTION_VALUE
Should be no comma before "and".

removed

STATS_CC_SESSION_ERROR
Same comments as STATS_HTTPD_CC_SESSION_ERROR

ok

STATS_RECEIVED_NEW_CONFIG
In light of the name, the text might be better as "the statistics module has received a new configuration from the configuration manager" rather that "configuration manager has sent ...". The latter seems to imply that this should be a message output by the configuration manager.

except that it's logged as [date] b10-stats.stats INFO, but ok, changed it.

STATS_RECEIVED_REMOVE_COMMAND
Can we explain what a "remove" command is and what is the "given name"? Also, does this have a permanent effect (i.e. when I run it, I can never add given name back)?

I kept it vague because i didn't understand why it is there in the first place. Added some more text;
"It will not appear in statistics reports until it appears in a statistics update from a module again."

STATS_RECEIVED_RESET_COMMAND
"reset all collected statistics": to what? Would be better to say that the reset command clears the stored statistics?

interesting, I've always considered 'reset' without 'to' to mean 'original state' (though i just noticed it sets boot_time to the epoch, guess there is no real distinction yet as to what items should be reset).

Anyhew I made it "clears, until it receives an update from the modules again" (note that this is a description of the current behaviour, not my consent to said behaviour, since i think it's wrong)

STATS_RECEIVED_SHUTDOWN_COMMAND
Should be no comma before "and".

removed

STATS_RECEIVED_STATUS_COMMAND
I feel that "I'm alive" is a bit flippant. Assuming that the only status that can be returned is "I'm alive", perhaps something like "The statistics module has received a command asking it to report its status. It will return a response indicating that it is running normally."

ok

STATS_RECEIVED_UNKNOWN_COMMENT
Should be no comma before "and".

english is weird :p removed

STATS_SEND_STATS_REQUEST_BOSS
If this is the only request that can be sent to Boss, perhaps just "STATS_SEND_REQUEST_BOSS" would be enough?

ok

Being pedantic here, The word "statistics" refers to a quantity (such as the mean or median) calculated from a set of data. Is the statistics module requesting Boss to send it this information, or is it requesting Boss to send it the raw counters from which it will calculate appropriate statistics?

removed 'data' from the message, and changed 'statistics data' to 'its data'.

STATS_STOPPED_BY_KEYBOARD
Perhaps add "(Obviously, this can only happen if the statistics daemon is being run from a terminal.)"

no :p

comment:9 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

comment:10 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Looks OK, please merge.

comment:11 Changed 9 years ago by jelte

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

Thanks, merged.

Note: See TracTickets for help on using tickets.