|
|
Created:
11 years, 4 months ago by jgc Modified:
11 years, 2 months ago Reviewers:
CC:
rsc, jgc_jgc.org, 0xjnml, mikio, bradfitz, golang-dev Visibility:
Public. |
Descriptionlog/syslog: correct message format
The syslog implementation was not correctly implementing the
traditional syslog format because it had a confused notion of
'priority'. syslog priority is not a single number but is, in
fact, the combination of a facility number and a severity. The
previous Go syslog implementation had a single Priority that
appeared to be the syslog severity and no way of setting the
facility. That meant that all syslog messages from Go
programs appeared to have a facility of 0 (LOG_KERN) which
meant they all appeared to come from the kernel.
Also, the 'prefix' was in fact the syslog tag (changed the
internal name for clarity as the term tag is more widely used)
and the timestamp and hostname values were missing from
messages.
With this change syslog messages are generated in the correct
format with facility and severity combined into a priority,
the timestamp in RFC3339 format, the hostname, the tag (with
the PID in [] appened) and the message.
The format is now:
<PRI>1 TIMESTAMP HOSTNAME TAG[PID]: MSG
The TIMESTAMP, HOSTNAME and PID fields are filled in
automatically by the package. The TAG and the MSG are supplied
by the user. This is what rsyslogd calls TraditionalFormat and
should be compatible with multiple systems.
Patch Set 1 #Patch Set 2 : diff -r 03a6b8c9c396 https://code.google.com/p/go #Patch Set 3 : diff -r 03a6b8c9c396 https://code.google.com/p/go #Patch Set 4 : diff -r c200281fac50 https://code.google.com/p/go #
Total comments: 6
Patch Set 5 : diff -r c200281fac50 https://code.google.com/p/go #
Total comments: 25
Patch Set 6 : diff -r c200281fac50 https://code.google.com/p/go #MessagesTotal messages: 24
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I'm sorry, but the Go 1 compatibility promise means we cannot remove existing types. functions, constants, global variables, methods, or exported struct fields. We can add new ones if necessary. So while we may have gotten what's there wrong, we're stuck with it. One possibility is to make the Go numbers different from the raw system numbers, so that we can treat Priority as a merged space, with constants that are half facility and half severity, and then split them (and remap to system numbers) during the actual call. const ( LOG_EMERG Priority = iota LOG_ALERT LOG_CRIT LOG_ERR LOG_WARNING LOG_NOTICE LOG_INFO LOG_DEBUG ) const ( LOG_USER Priority = iota<<16 LOG_KERN LOG_MAIL LOG_DAEMON ... ) I've swapped LOG_USER and LOG_KERN so that LOG_USER is zero. That will mean that calls using, say, LOG_ERR end up automatically using LOG_USER|LOG_ERR, but if you want to do something different you can do LOG_DEBUG|LOG_MAIL, say.
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 2:35 PM, <rsc@golang.org> wrote: > One possibility is to make the Go numbers different from the raw system > numbers, so that we can treat Priority as a merged space, with constants > that are half facility and half severity, and then split them (and remap > to system numbers) during the actual call. > > const ( > LOG_EMERG Priority = iota > LOG_ALERT > LOG_CRIT > LOG_ERR > LOG_WARNING > LOG_NOTICE > LOG_INFO > LOG_DEBUG > ) > > const ( > LOG_USER Priority = iota<<16 > LOG_KERN > LOG_MAIL > LOG_DAEMON > ... > ) > > I've swapped LOG_USER and LOG_KERN so that LOG_USER is zero. That will > mean that calls using, say, LOG_ERR end up automatically using > LOG_USER|LOG_ERR, but if you want to do something different you can do > LOG_DEBUG|LOG_MAIL, say. OK. I can certainly modify my code to do that. It'll require a little documentation so that people know how to set the priority in New/NewLogger but should work. The only thing I don't like is that you've swapped LOG_USER and LOG_KERN. I don't think there's any need to change the default behaviour. If we leave LOG_KERN as 0 then people using the package will see no difference in behaviour. I will modify and resubmit. John.
Sign in to reply to this message.
> but should work. The only thing I don't like is that you've swapped LOG_USER > and LOG_KERN. I don't think there's any need to change the default > behaviour. If we leave LOG_KERN as 0 then people using the package will see > no difference in behaviour. But the current behavior is a bug, right? I mean, Go programs are not the kernel. We could also just drop LOG_KERN entirely. Russ
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 2:58 PM, Russ Cox <rsc@golang.org> wrote: > > but should work. The only thing I don't like is that you've swapped > LOG_USER > > and LOG_KERN. I don't think there's any need to change the default > > behaviour. If we leave LOG_KERN as 0 then people using the package will > see > > no difference in behaviour. > > But the current behavior is a bug, right? I mean, Go programs are not > the kernel. We could also just drop LOG_KERN entirely. > It's a bug in that Go programs are not (currently) the kernel (although one day they might be). The trouble with making the change that you propose is that programs that are using the syslog facility as is will have worked around this (we did this internally initially with a separate syslogd instance just for grabbing data from Go programs). I think it's well within the goal of backwards compatibility to not change that. Going forward people will do syslog.New(LOG_DEBUG | LOG_USER, "foo"). John.
Sign in to reply to this message.
> I think it's well within the goal of backwards compatibility to not change > that. Going forward people will do syslog.New(LOG_DEBUG | LOG_USER, "foo"). As long as you change the arguments used in the Emerg, Alert, Crit, etc wrappers, sure. Russ
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 3:04 PM, Russ Cox <rsc@golang.org> wrote: > > I think it's well within the goal of backwards compatibility to not > change > > that. Going forward people will do syslog.New(LOG_DEBUG | LOG_USER, > "foo"). > > As long as you change the arguments used in the Emerg, Alert, Crit, > etc wrappers, sure. > So, those functions only let the user specify the severity (implied by the function name). I think the right way to do that is to inherit the facility from the facility set up in New/NewLogger (just as we do today with the default of kernel). That actually allows a rather nice usage where the facility is supplied in the New and the severity by calling the right function: w, err := syslog.New(LOG_USER, "foo") w.Emerg("bad thing") would send a syslog message to the user facility with severity LOG_EMERG. John.
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 4:02 PM, John Graham-Cumming <jgc@jgc.org> wrote: > I think it's well within the goal of backwards compatibility to not change > that. I don't think that the Go 1 promise covers keeping bugs "backward compatible". It's a pity that instead of an early bug report a workaround code was somewhere put to production, but IMO any bug has to be fixed, not preserved. -j
Sign in to reply to this message.
How about another option; making new packet go.net/syslog that implements RFC 5424 The Syslog Protocol? On Tue, Nov 27, 2012 at 12:07 AM, John Graham-Cumming <jgc@jgc.org> wrote: > On Mon, Nov 26, 2012 at 3:04 PM, Russ Cox <rsc@golang.org> wrote: >> >> > I think it's well within the goal of backwards compatibility to not >> > change >> > that. Going forward people will do syslog.New(LOG_DEBUG | LOG_USER, >> > "foo"). >> >> As long as you change the arguments used in the Emerg, Alert, Crit, >> etc wrappers, sure. > > > So, those functions only let the user specify the severity (implied by the > function name). I think the right way to do that is to inherit the facility > from the facility set up in New/NewLogger (just as we do today with the > default of kernel). > > That actually allows a rather nice usage where the facility is supplied in > the New and the severity by calling the right function: > > w, err := syslog.New(LOG_USER, "foo") > w.Emerg("bad thing") > > would send a syslog message to the user facility with severity LOG_EMERG. > > John. >
Sign in to reply to this message.
Hello rsc@golang.org, jgc@jgc.org, 0xjnml@gmail.com, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I've updated the CL so that it is backwards-compatible and uses ORing of the severity and facility as you suggest. John.
Sign in to reply to this message.
https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:28: delete this line, otherwise the preceding comment isn't attached to the type in godoc. https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:68: ) no need to end and close this const block. just replace these two lines with a blank line. https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:110: if priority < 0 || priority > (LOG_LOCAL7 | LOG_DEBUG) { no need for the parens. gofmt should make it look correct once you remove them. https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:134: return &Writer{priority, tag, hostname, conn}, err now that you have two struct members of the same type (string and string), don't list them positionally here. name the struct fields instead. and don't then hide a dangling err at the end. I'd if err != nil and return early with a nil Writer in that case, and then you can dangle a nil at the end of your return statement. https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:152: // by the call to New end in a period, here and the 4 or so cases below. https://codereview.appspot.com/6782118/diff/11001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:213: tagWithPid := fmt.Sprintf("%s[%d]", tag, os.Getpid()) why not just put this printf into the next printf?
Sign in to reply to this message.
Hello rsc@golang.org, jgc@jgc.org, 0xjnml@gmail.com, mikioh.mikioh@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I have incorporated all bradfitz's suggested changes. John.
Sign in to reply to this message.
Getting close, thanks. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:22: // severity. To set the facility and severity merge bitwise OR a the "To set..." can be shortened to "For example, LOG_ALERT | LOG_FTP ..." https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:24: // priority message from the FTP facility. If no severity is included s/If no.../The default severity is LOG_EMERG; the default facility is LOG_KERN. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:33: Delete blank line. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:34: // The syslog severity We're in package syslog so this can shorten to // Severity. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:49: Delete blank line. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:50: // The syslog facility // Facility. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:54: LOG_KERN Priority = iota << 3 I'd like to avoid changing these constants in the future. Let's use 8 or 16 and then there is plenty of room to grow on both sides. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:67: LOG_LOCAL0 Priority = (iota + 16) << 3 iota here is 12, not 0, so LOG_LOCAL0 is 28<<3. Is that what you intend, or were you going for 16<<3? If the latter, you might do LOG_FTP _ // unused _ // unused _ // unused _ // unused LOG_LOCAL0 LOG_LOCAL1 ... The _ lines bump iota but do not declare anything (because _ never declares anything). https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:95: // specifies the facility and default severity. The default severity Let's avoid explaining facility and severity multiple times. The docs on Priority make it clear. Also, I think we can leave the special case descriptions of Emerg, Alert, Crit, etc to those method comments, so I think we can just leave this comment as it was (the rewrite lost the information about each write generating a message): // New establishes a new connection to the system log daemon. // Each write to the returned writer sends a log message with // the given priority and prefix. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:115: hostname, err := os.Hostname() You don't appear to use err here. Make that clear. hostname, _ := os.Hostname() var err error below https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:145: // Emerg logs a message using the LOG_EMERG severity and facility set // Emerg logs a message with severity LOG_EMERG, ignoring the severity passed to New. etc https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:226: // system log service with the specified facility and severity. The This comment can be reverted too.
Sign in to reply to this message.
https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:22: // severity. To set the facility and severity merge bitwise OR a On 2012/11/26 17:07:59, rsc wrote: > the "To set..." can be shortened to "For example, LOG_ALERT | LOG_FTP ..." Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:24: // priority message from the FTP facility. If no severity is included On 2012/11/26 17:07:59, rsc wrote: > s/If no.../The default severity is LOG_EMERG; the default facility is LOG_KERN. Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:33: On 2012/11/26 17:07:59, rsc wrote: > Delete blank line. Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:34: // The syslog severity On 2012/11/26 17:07:59, rsc wrote: > We're in package syslog so this can shorten to > > // Severity. > Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:49: On 2012/11/26 17:07:59, rsc wrote: > Delete blank line. Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:50: // The syslog facility On 2012/11/26 17:07:59, rsc wrote: > // Facility. Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:54: LOG_KERN Priority = iota << 3 On 2012/11/26 17:07:59, rsc wrote: > I'd like to avoid changing these constants in the future. Let's use 8 or 16 and > then there is plenty of room to grow on both sides. Doing that would mean that the package would have to internally translate from these values to the actual values used in syslog itself. Given that these values are drawn from /usr/include/sys/syslog.h I think it's better and clearer to leave them as is. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:67: LOG_LOCAL0 Priority = (iota + 16) << 3 That's a mistake I introduced following a change suggested by bradfitz. I have corrected it and added a test case. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:95: // specifies the facility and default severity. The default severity On 2012/11/26 17:07:59, rsc wrote: > Let's avoid explaining facility and severity multiple times. The docs on > Priority make it clear. Also, I think we can leave the special case descriptions > of Emerg, Alert, Crit, etc to those method comments, so I think we can just > leave this comment as it was (the rewrite lost the information about each write > generating a message): > > // New establishes a new connection to the system log daemon. > // Each write to the returned writer sends a log message with > // the given priority and prefix. Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:115: hostname, err := os.Hostname() On 2012/11/26 17:07:59, rsc wrote: > You don't appear to use err here. Make that clear. > hostname, _ := os.Hostname() > var err error below Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:145: // Emerg logs a message using the LOG_EMERG severity and facility set On 2012/11/26 17:07:59, rsc wrote: > // Emerg logs a message with severity LOG_EMERG, ignoring the severity passed to > New. > > etc Done. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:226: // system log service with the specified facility and severity. The On 2012/11/26 17:07:59, rsc wrote: > This comment can be reverted too. Done.
Sign in to reply to this message.
Hello rsc@golang.org, jgc@jgc.org, 0xjnml@gmail.com, mikioh.mikioh@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Looks good except that I'd still like to expand the shift count (or understand why we can't). Thanks. https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782118/diff/3004/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:54: LOG_KERN Priority = iota << 3 On 2012/11/26 17:24:08, jgc wrote: > On 2012/11/26 17:07:59, rsc wrote: > > I'd like to avoid changing these constants in the future. Let's use 8 or 16 > and > > then there is plenty of room to grow on both sides. > > Doing that would mean that the package would have to internally translate from > these values to the actual values used in syslog itself. Given that these values > are drawn from /usr/include/sys/syslog.h I think it's better and clearer to > leave them as is. I am not sure we're talking about the same thing. I am talking about using <<8 or <<16 instead of <<3 in the encoding of Severity,Facility -> Priority. Since I don't believe the two get merged in syslog.h I don't believe the <<3 is from there.
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 8:46 PM, <rsc@golang.org> wrote: > Looks good except that I'd still like to expand the shift count (or > understand why we can't). > > Thanks. > >> >> [snip] > > > I am not sure we're talking about the same thing. I am talking about > using <<8 or <<16 instead of <<3 in the encoding of Severity,Facility -> > Priority. Since I don't believe the two get merged in syslog.h I don't > believe the <<3 is from there. > > https://codereview.appspot.**com/6782118/<https://codereview.appspot.com/6782... > syslog itself (and the RFC's specify this) makes the priority from facility << 3 | severity. Given that we don't have separate concepts of facility and severity in the Go syslog package because we've encoded them both in the Priority const it makes most sense to me to use exactly the same method as syslog. That way there's no internal translation needed to get the priority value that is sent to syslog and the user can simply OR together a facility and severity drawn from the Priority. The priority value passed to New/NewLogger is exactly what syslog needs. Of course, we could change the encoding in an arbitrary manner to allow space, but I don't think that's particularly useful given that for us to need extra space (say for more severities) syslog itself would have to change encoding. John.
Sign in to reply to this message.
Wow. I had no idea that was the RFC encoding when I suggested it earlier today. Great.
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 9:55 PM, Russ Cox <rsc@golang.org> wrote: > Wow. I had no idea that was the RFC encoding when I suggested it > earlier today. Great. > And there I was thinking "what a brilliant idea" when I saw your suggestion :-) John.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=a041b45cc418 *** log/syslog: correct message format The syslog implementation was not correctly implementing the traditional syslog format because it had a confused notion of 'priority'. syslog priority is not a single number but is, in fact, the combination of a facility number and a severity. The previous Go syslog implementation had a single Priority that appeared to be the syslog severity and no way of setting the facility. That meant that all syslog messages from Go programs appeared to have a facility of 0 (LOG_KERN) which meant they all appeared to come from the kernel. Also, the 'prefix' was in fact the syslog tag (changed the internal name for clarity as the term tag is more widely used) and the timestamp and hostname values were missing from messages. With this change syslog messages are generated in the correct format with facility and severity combined into a priority, the timestamp in RFC3339 format, the hostname, the tag (with the PID in [] appened) and the message. The format is now: <PRI>1 TIMESTAMP HOSTNAME TAG[PID]: MSG The TIMESTAMP, HOSTNAME and PID fields are filled in automatically by the package. The TAG and the MSG are supplied by the user. This is what rsyslogd calls TraditionalFormat and should be compatible with multiple systems. R=rsc, jgc, 0xjnml, mikioh.mikioh, bradfitz CC=golang-dev http://codereview.appspot.com/6782118 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|