Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added basic syslog support #486

Closed
wants to merge 3 commits into from
Closed

Added basic syslog support #486

wants to merge 3 commits into from

Conversation

ereOn
Copy link

@ereOn ereOn commented Mar 7, 2017

This PR is an attempt to add basic syslog support as requested in #333.

It does so by relying on the very recent log/level package.

Example usage:

w, _ := syslog.New(syslog.LOG_WARNING, "foo")
defer w.Close()
logger := level.NewSyslogLogger(w, log.NewLogfmtLogger)
level.Error(logger).Log("name", "jake")

@peterbourgon
Copy link
Member

peterbourgon commented Mar 7, 2017

Thanks, interesting approach. I like the mapping of the 4 levels to the the corresponding syslog priorities. I don't like the fact that it's defined in package level, nor that it uses a factory parameter to manufacture new loggers.

I guess I'd want to see the constructor to a SyslogLogger take a *syslog.Writer, which has a default priority for log messages built-in already. Then, I'd want to see the Log method introspect on keyvals to pick which Alert/Crit/Debug/Info/... method to ultimately call on the syslog.Writer, defaulting to Write if no matching level key is found. I'd want users to be able to declare and define that mapping, and I'd want the mapping you've hard-coded (the 4 levels in package level) to be a sensible default that users can opt-in to. I wouldn't be surprised if this is enough complexity that it warrants its own package go-kit/kit/log/syslog, but perhaps not.

Does all of that make sense?

@ereOn
Copy link
Author

ereOn commented Mar 7, 2017

@peterbourgon It does make a lot of sense. Thanks for the feedback.

I'd actually like to propose another option first where there is no explicit syslog logger, but rather a new kind of io.Writer that is able to write to syslog. I'll show you what I mean and you can tell me what you think about it. If it's not good, I'll implement it the way you just suggested.

@ereOn
Copy link
Author

ereOn commented Mar 7, 2017

@peterbourgon Here is my updated PR. It should give you a gist of the idea:

  • I added the concept of SpecializedWriter which is an interface that, provided a keyvals ...interface{} returns a specialized io.Writer instance.
  • I added a specializedWriter method that makes a resolution on a io.Writer or falls back to returning the io.Writer itself if it doesn't implement SpecializedWriter, thus ensuring backward-compatibility.
  • All the existing loggers (LogFmt and JSON explicitely) use specializedWriter.
  • I added a new SyslogWriter that implements SpecializedWriter and by default, reads the level field to determine which syslog priority to use.
  • The syslog priority resolution can be overriden by users with an option in the SyslogWriter constructor.

I think it makes sense to do it that way because from my understanding, loggers implement how the logs are formatted and writers define where/how they are written. Syslog does not impose any format in the log messages themselves, so this implementation allows users to use whatever log formatter they want (LogFmt or JSON for instance).

New example usage:

// This is from golang's builtin syslog package.
w, _ := syslog.New(syslog.LOG_WARNING, "foo")
defer w.Close()

// kitsyslog is the new `log/syslog` package in go-kit
logger := log.NewLogfmtLogger(kitsyslog.SyslogWriter(w))
level.Error(logger).Log("name", "jake")

log/syslog.go Outdated
func (w syslogWriter) GetSpecializedWriter(keyvals ...interface{}) io.Writer {
priority := w.selector(keyvals...)

switch priority {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A complete implementation will of course handle all supported syslog.Priority values.

@peterbourgon
Copy link
Member

peterbourgon commented Mar 15, 2017

All the existing loggers (LogFmt and JSON explicitely) use specializedWriter.

Yeah, this isn't gonna fly, sorry. It's not reasonable to have every other logger implementation wrap its underlying writer at serialization time with a decorator — neither from a design perspective nor (I'm guessing) a performance perspective.

As far as I can see, the design challenges here boil down to two competing requirements. First, the Syslog thing needs to introspect on the keyvals, in order to determine a priority, which means it most naturally needs to implement the Logger interface. But second, the Syslog thing is ultimately going to be a destination for other logger implementations, which means it most naturally needs to implement io.Writer. I'm not sure how to square this circle, except to say the current approach isn't the right one.

Speculating: what about a type that implements both Logger and io.Writer, and can be wired in as both a decorator and a destination, shuttling state (i.e. priority) from the decorator part to the writer part with each invocation? e.g.

sw := log.NewSyslogWriter(...)

var logger log.Logger
logger = log.NewLogfmtLogger(sw) // <--to the writer---------------.
logger = log.With(logger, "ts", log.DefaultTimestampUTC) //        |
logger = sw.Decorate(logger) // capture level somehow and feed it--'

If you don't Decorate your Logger stack, that's fine: you'll get some default priority.

@ChrisHines
Copy link
Member

Disclaimer: I haven't looked at the code in this PR yet, so forgive me if this is off base.

I think it makes sense to do it that way because from my understanding, loggers implement how the logs are formatted and writers define where/how they are written. Syslog does not impose any format in the log messages themselves, so this implementation allows users to use whatever log formatter they want (LogFmt or JSON for instance).

That sounds like the same problem that we had with term.NewColorLogger. Take a look at its approach to composing a func(io.Writer) log.Logger and io.Writer to support arbitrary log formats with other functionality.

@ereOn
Copy link
Author

ereOn commented Mar 15, 2017

@peterbourgon & @ChrisHines Okay. I'll look at that and I see how I can follow the same design.

Looking back at the derivative io.Writer classes, I'm not too fond either. It just isn't "simple" enough.

I'll keep you posted.

@mingan
Copy link
Contributor

mingan commented Jul 1, 2017

Any progress on the code, @ereOn, or should I take crack at it?

@ereOn
Copy link
Author

ereOn commented Jul 1, 2017

@mingan You can take over if you like. I sadly am swamped in many other things to do these days.

@mingan mingan mentioned this pull request Jul 14, 2017
@peterbourgon
Copy link
Member

Closing in favor of #574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants