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

Add a syslog encoder and sink #96

Open
lysu opened this issue Jul 5, 2016 · 13 comments
Open

Add a syslog encoder and sink #96

lysu opened this issue Jul 5, 2016 · 13 comments

Comments

@lysu
Copy link

lysu commented Jul 5, 2016

now zap can write to stderr and stdout...

Is any plan to support syslog?

@akshayjshah
Copy link
Contributor

Eventually, yes - supporting syslog will require both a syslog encoder (similar to zap's current JSON encoder) and a WriteSyncer.

We're definitely not going to get to this until after a 1.0 release, though.

@akshayjshah akshayjshah changed the title How can zap support syslog Add a syslog encoder and sink Jul 6, 2016
@skypacer210
Copy link

Hi @akshayjshah
Dose it mean that zap can't write log to file? If zap support write to file, how to use it? Thanks a lot.

@prashantv
Copy link
Collaborator

@skypacer210 This issue is not related to writing to a file, but specifically to the syslog interface.

You can write to a file using zap, take a look at zap.Output option. The os.File type satisfies this interface so you can open a file and pass it in as the output.

I've filed #100 so we have an example to point to for file logging.

@skypacer210
Copy link

@akshayjshah Thanks a lot!

@skypacer210
Copy link

@akshayjshah Do zap support logging with rotation?

@prashantv
Copy link
Collaborator

@skypacer210 Please look at existing documentation and issues, there is a separate issue about this: #98. As I mentioned earlier, this issue is not related to writing to a file, but about the syslog interface, so let's move the discussion of logging to file away from this issue.

@skypacer210
Copy link

@prashantv Got it, thanks.

@jeanlucmongrain
Copy link

jeanlucmongrain commented Sep 8, 2016

I actually implemented a nearly completed Syslog friendly encoder (mostly just a slightly modified JSON encoder) and a syslog writer.

But I can't finish it because zap.Encoder.Encoder interface WriteEntry(io.Writer, string, Level, time.Time) error function first argument io.Writer is not enough.

The problem is: syslog writer need to know the zap.Level and not just the []byte of the encoded log entry. Syslog must specify priority and level on each new log entry.

To achieve this I suggest create the new following interface:

type LogWriter interface {
        Write([]byte, Level) (int, error)
}

and use it instead of io.Writer in WriteEntry.

For writers that don't need the level such to use os.Stdout as a LogWriter, we can use that wrapper:

type wrappedWriter struct {
    w io.Wrapper
}

func (w *wrappedWriter) Write(p []byte, _ Level) (int, error) {
    return w.Write(p)
}

func WrapIgnoreLevel(w io.Wrapper) LogWriter {
    return &wrappedWriter{w}
}

If this change to zap internal API is approved, I can finish implement syslog support.

@prashantv
Copy link
Collaborator

prashantv commented Sep 16, 2016

Adding Level seems like a change that's very particular to this use-case. I think passing a struct that contains message, time, and level would be cleaner, and this same struct could also be used in the Encoder interface.

This struct is almost the same as Entry, so perhaps we should reduce what Entry is responsible for, and make the following interface changes:

  • Entry no longer contains encoder or fields, it would only contain message, level, and time.
  • Hook changes from func(*Entry) to func(*Entry, Keyvalue)
  • Encoder's WriteEntry changes from WriteEntry(io.Writer, string, Level, time.Time) to WriteEntry(io.Writer, Entry) (or possibly *Entry for efficiency).

@akshayjshah
Copy link
Contributor

@bclermont Thanks for putting in the time to prototype this!

I'd also like to make an interface changes generic enough that we have a fighting chance of supporting other encodings and sinks down the road. Modifying Entry makes sense to me.

However, I would like to preserve the separation of encoders and outputs. While the syslog format and syslog outputs are pretty tightly coupled, this sort of thing is broadly useful - for example, it's reasonable to support different files per log level (or per name, if we implement #130). To support those use cases cleanly, I do think we'll need to rethink the interface that outputs provide; io.Writer doesn't receive enough information to do the job.

@bclermont This discussion might be a bit easier if you open a PR with your code, even if it's incomplete - at least then we'll have a concrete implementation to look at.

@akshayjshah akshayjshah modified the milestone: 1.0.0 Jan 3, 2017
@tchap
Copy link

tchap commented Mar 20, 2017

In case anybody is interested in syslog, you might want to check https://github.com/tchap/zapext/blob/master/zapsyslog/core.go ;-)

Exemplar usage can be found here: https://github.com/tchap/zapext/blob/master/zapsyslog/example/main.go

@avidak
Copy link

avidak commented Sep 27, 2017

I implemented syslog writer in my own application so I made pull request if you are interested in using it in your library

#506

akshayjshah pushed a commit that referenced this issue Apr 13, 2018
As part of the discussion on #96,
it's clear that we need to make it easier for third-party packages to
extend zap's functionality. Some of that requires new APIs, but we'll
also need a way to refer people from the core zap package to the various
extensions.

This PR adds an FAQ entry that lists the extensions we're aware of. It's
currently a little hard to find (the GoDoc includes a link to the FAQ,
but it's not very prominent). We'll address that as part of a
documentation revamp later on.
@akshayjshah
Copy link
Contributor

akshayjshah commented Apr 13, 2018

Awesome - thanks, @avidak. Clearly, we've been thinking about this a lot internally without posting updates on this issue - sorry, everyone!

I don't think that it's a good idea to include this functionality inside zap itself. I do agree that it's important, and clearly many of you need it. However, we don't use syslog internally, and none of the zap maintainers have any experience with it. We're committed to supporting this project, including fixing bugs...which will be very difficult for anything syslog-related. As a small example, none of us are certain whether it's okay to send msgpack-encoded (or other binary) logs to syslog.

To make sure that the syslog integration is well-supported and works correctly, it should be maintained by someone who actually uses it and understands syslog. @tchap has already written such an extension, which I'm happy to send users to. I've added a table of extensions to zap's FAQ in #576; as we revamp the zap docs, that table will become more prominent.

Again, sorry for the long silence on this issue. I hope that this approach ends up being good for everyone: users get a supported, battle-tested syslog integration, and zap itself remains understandable to the core maintainers.

I'll leave this issue open for a bit, in case we need a bit more discussion.

akshayjshah pushed a commit that referenced this issue Apr 13, 2018
As part of the discussion on #96,
it's clear that we need to make it easier for third-party packages to
extend zap's functionality. Some of that requires new APIs, but we'll
also need a way to refer people from the core zap package to the various
extensions.

This PR adds an FAQ entry that lists the extensions we're aware of. It's
currently a little hard to find (the GoDoc includes a link to the FAQ,
but it's not very prominent). We'll address that as part of a
documentation revamp later on.
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
As part of the discussion on uber-go#96,
it's clear that we need to make it easier for third-party packages to
extend zap's functionality. Some of that requires new APIs, but we'll
also need a way to refer people from the core zap package to the various
extensions.

This PR adds an FAQ entry that lists the extensions we're aware of. It's
currently a little hard to find (the GoDoc includes a link to the FAQ,
but it's not very prominent). We'll address that as part of a
documentation revamp later on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants