BIND 10 #758: Conversion of bind10 program to use new logging interface

[BIND 10 Development]

#758: Conversion of bind10 program to use 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      |
-------------------------------------+-------------------------------------
Changes (by jinmei):

 * owner:  jinmei => jelte


Comment:

 Replying to [comment:9 jelte]:

 I don't have any more blocking issues.  Please feel free to merge.

 I wanted to make further comments on some selected points, though,
 which follow:

 > > '''specifics'''
 > >
 > > - shouldn't this better be "bind10", "boss", or "bob" etc?
 [...]
 > But come to think of it, when looking from the administrators point of
 view, there may be a deeper problem: i think bind10[.py] has the wrong
 name.
 > So i'm thinking that we should actually change that too, and make
 'bind10' a script that calls b10-boss, so this will be 'just another'
 module in the process list where all the others already start with b10-.
 This may be related to an effort of getting 'internal' and 'external'
 names of modules, processes and config consistent, but in the case of
 bind10.py there is even more of a disconnect than for the others right
 now.
 >
 > Changed the root logger name to 'b10-boss' and the actual logger to
 'boss' (being closest to the current command/configuration name of the
 module).

 I don't have an objection to the change, and I tend to agree that
 bind10[.py] is not a good (if not "wrong") name.  I have one note,
 though, that (if I remember it correctly) we add the b10- prefix to
 the child programs so that we can easily find them by, e.g., ps +
 grep.  It doesn't apply to the "boss" program, and we intentionally
 didn't add "b10-" to it.  Probably you knew that but chose to make log
 outputs consistent, in which case I don't oppose to that (actually I
 don't have a strong opinion on this point).  But we'll probably need
 comprehensive documentation for logging in general, and we should
 describe these conventions there.

 In any case, for the scope of this ticket, I won't request further
 change on this point.

 > > - BIND10_START_AS_NON_ROOT: If we have a ticket for the socket
 > >   creator, I'd add a note to it about this description:
 >
 > ok, there's two tickets, neither of which is 'the ticket' for inclusion
 (#800 and #801), but i added them anyway.

 Actually, what I meant was to add comments *to* the tickets that when
 they are done the log message for the boss will have to be updated.
 I'd leave it to you whether to keep the (added) reference to the
 ticket numbers in the log message.

 > > - BIND10_STARTING_PROCESS_PORT: maybe we should change '%2:%3' to
 > >   '%2#%3' as we discussed on jabber (and I saw a ticket about this)
 >
 > Is that a common notation? I don't think i've ever seen it used (well,
 apart from bind9, apparently). I realize addr:port is ambiguous if it's
 ipv6 (where [addr]:port is usually used), but # seems quite unusual to me.
 Changed it to # anyway, in a step towards consistency.

 See #1086.  The use of # is purely for compatibility with BIND 9.  I
 agree the format is uncommon per se, and I'm okay with reconsidering
 it (if other points are neutral, consistency/compatibility with BIND 9
 would generally be good, but when a particular behavior of it is
 considered a bad practice we should rather change it).

 > > - in my test run (with -v, from source), I couldn't find logs in
 > >   log_starting().

 > no i originally set them to DEBUG, since i didn't like the 3 terminal
 screens of STARTING/STARTED messages. So they are not printed by default
 (since default is INFO). Changed STARTING message to severity INFO, and i
 left STARTED at DEBUG, to make it the output a bit less. But perhaps we
 should remove the STARTED log completely, and only log something if
 starting failed.

 Ah, okay.  Hmm, I don't have a strong opinion about info vs debug, but
 if they are not so frequent (I suspect not), I'd slightly prefer info.
 Also, considering the option of starting a selected subset of
 processes, I think logging the (successful) event would be still
 useful.

 > > - BIND10_SEND_SIGTERM: I'd use "notice" (if we have it) because it's
 > >   not an unexpected case even if not considered an error.  also, you
 > >   may want to refer to BIND10_SHUTDOWN to explain when this happens.
 > >   Same for BIND10_SEND_SIGKILL.
 >
 > there's no notice, so info seems closest (which it already is). There's
 only one case where this isn't unexpected, and that is for msgq, btw.

 Okay about the change.  As for the 'btw' part, I actually meant
 "because it's an unexpected case" (although I'm still not sure if I
 understand your btw comment).

 > > - BIND10_PROCESS_ENDED_WITH_EXIT_STATUS: as I said on jabber in a
 > >   different context I personally don't like to repeat the same content
 > >   in multiple places (most of it is a duplicate of NO_EXIT_STATUS),
 > >   especially for text this long.  But understanding this may be a
 > >   matter of opinion, I'd leave it to you.
 >
 > I actually disagree with the general premise, but in this case a
 reference will do (due to the way these messages are tied together in the
 way that they are conditionally called)

 Okay.  But I'd like to clarify one thing: my major point in this
 context is "don't repeat yourself".  Using a reference to another log
 ID is just one possible solution to the problem.  It was not clear
 whether you agree with the major point but are just not comfortable
 with the solution using the reference, or you disagree with the major
 point in the first place.  If it's the former, we could consider other
 solutions (e.g., introducing a macro block that can be shared by
 multiple log messages).  If it's the latter, that's fine, too; I
 understand that may be a matter of opinion.

 > > - BIND10_INVALID_USER: very minor point, but the use of '' seems to be
 > >   inconsistent.  Also, 'invalid user' seems to be too simple.  Maybe
 > >   something like 'changing user to %1 failed'?
 >
 > 'failed' seems too general to me, imo it should be either invalid user
 or unknown user

 Okay.

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