Conversation
|
Hi, any updates on this? |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
For the most part, I like this approach. A couple of tiny changes and I'll be happy to add it. I apologize profusely for the delay. If the original author doesn't show up in a little bit, I'll make the changes myself.
log/syslog/syslog_logger.go
Outdated
| "sync" | ||
| ) | ||
|
|
||
| // PrioritySelector inspects list of keyvals and select a syslog priority |
There was a problem hiding this comment.
PrioritySelector inspects the list of keyvals, and selects a syslog priority.
| "github.com/go-kit/kit/log" | ||
| "fmt" | ||
| "github.com/go-kit/kit/log/level" | ||
| ) |
There was a problem hiding this comment.
Can we do proper import grouping, i.e.
import (
// stdlib
// go-kit
)
|
@ChrisHines Any blockers that you see? |
ChrisHines
left a comment
ChrisHines
left a comment
There was a problem hiding this comment.
I don't have any experience with the stdlib log/syslog package, or syslog in general, so I don't have any opinion on the use of syslog here.
I like the exported API presented by this package. It matches the style of the other go-kit/log packages. 👍
I found an edge case that will panic which should be fixed.
Otherwise just a few nits and some readability bike shedding.
log/syslog/syslog_logger.go
Outdated
| func defaultPrioritySelector(keyvals ...interface{}) gosyslog.Priority { | ||
| for i := 0; i < len(keyvals); i += 2 { | ||
| if keyvals[i] == level.Key() { | ||
| if v, ok := keyvals[i+1].(level.Value); ok { |
There was a problem hiding this comment.
This line may panic if len(keyvals) is odd and the last element == level.Key().
log/syslog/syslog_writer.go
Outdated
| gosyslog "log/syslog" | ||
| ) | ||
|
|
||
| // SyslogWriter is an interface wrapping stdlib syslog Writer |
There was a problem hiding this comment.
Missing period (.) at end of comment.
log/syslog/syslog_writer.go
Outdated
|
|
||
| type syslogWriter struct { | ||
| gosyslog.Writer | ||
| } |
There was a problem hiding this comment.
type syslogWriter doesn't appear to be used anywhere, I think it can be removed.
log/syslog/syslog_logger.go
Outdated
| ) | ||
|
|
||
| // PrioritySelector inspects the list of keyvals and selects a syslog priority | ||
| type PrioritySelector func(keyvals ...interface{}) gosyslog.Priority |
There was a problem hiding this comment.
- Missing period (.) at end of comment.
- Do we need to emphasize that a PrioritySelector may not modify the contents of keyvals?
- For readability of the source code I would move to just before the
func PrioritySelectorOptiondeclaration.
log/syslog/syslog_logger.go
Outdated
|
|
||
| func (l *syslogLogger) putLoggerBuf(cb *loggerBuf) { | ||
| l.bufPool.Put(cb) | ||
| } |
There was a problem hiding this comment.
- s/cb/lb to be consistent with variable names in
getLoggerBuf. - I feel like it would help readability to move the
type loggerBuf,func (l *syslogLogger) getLoggerBuf, andfunc (l *syslogLogger) putLoggerBufdeclarations to after thefunc (l *syslogLogger) Logdeclaration because they are couple together more tightly than theOptionsrelated code. I find myself having to skip over theOptionsstuff a lot while reading the code.
log/syslog/syslog_writer.go
Outdated
| Info(string) error | ||
| Debug(string) error | ||
| } | ||
|
|
There was a problem hiding this comment.
I would prefer all of the code in this file be combined with syslog_logger.go and the combined file be renamed to syslog.go. I would put the type SyslogWriter declaration directly above the func NewSyslogLogger declaration.
|
I implemented all the comments apart from those phrased as questions. I rebased on top of master so tests from other subpackages pass as well. |
|
Thanks everyone, and sorry again for that inexcusably long delay. |
4 participants
This is another crack at #333. It incorporates the discussion from #486 but it shares very little code with it. In structure, it's very similar to term.NewColorLogger - it's so similar that
loggerBufcould be extracted and be used b both (but in which package should it live? it would have to be exported which might be confusing?).However, while working on this I realized that it breaks on one the 12 factor app principles. Including it in Go kit might be a promotion of a suboptimal approach, so I'm not sure it should be included at all.