BIND 10 #763: Conversion of statistics code to use the new logging interface

[BIND 10 Development]

#763: Conversion of statistics code to use the new logging interface
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110712
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  2.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jelte):

 Replying to [comment:7 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

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