Support for adding custom encoders

[jsternberg]

I would like a way to add custom encoders using the config here. There is an Encoding option here that allows both console and json to be used.

I had previously made a custom encoder from a previous development version of this library. While I haven't yet updated that library to work with the current release candidate, a hook that would allow me to add something like this into the library would be nice.

func NewLogfmtEncoder(cfg zapcore.EncoderConfig) zapcore.Encoder
        // instantiate the logfmt encoder here
}

func init() {
        zap.RegisterEncoder("logfmt", NewLogfmtEncoder)
}

It doesn't need to be very complicated. It can just be a map protected by a global read/write mutex (or not even that since loggers should be registered in init() functions) that panics whenever there is a duplicate. I'm thinking something similar to how database/sql allows additional database handlers.

[akshayjshah]

Let me ruminate on this a bit. I like the idea of making zap's built-in config extensible, but I don't particularly like the way that the built-in SQL package works - I'd really like to avoid head-scratchers like

import _ "github.com/foo/fancy-encoder"

If I can't come up with a better idea, though, I can certainly implement something similar to what you've suggested.

[akshayjshah]

@prashantv, @jcorbin, and @peter-edge - any thoughts on this? I like the idea of letting third-party packages contribute encodings, since it lets zap's config struct be a bit more extensible. I don't particularly love the blank imports, but I don't have any better ideas. Does anyone have objections to this plan?

If we're okay with it, I'd like to take the same approach to DurationEncoder, TimeEncoder, and LevelEncoder.

[bufdev]

This is pretty much what I do with lion https://github.com/peter-edge/lion-go/blob/master/proto/protolion.go#L53. As I go on, I generally prefer having an explicit Register function instead of using inithttps://github.com/peter-edge/dlog-go/blob/master/logrus/logrus.go#L27, but this is subjective. The init style is more widespread, but I rather this be explicit, so just a suggestion.

Init style:

// custom_encoder.go
package custom

func init() {
  zap.RegisterEncoder(...)
}

// main.go
import _ "github.com/org/custom"

Register style:

// custom_encoder.go
package custom

func Register() {
  zap.RegisterEncoder(...)
}

// main.go
func main() {
  custom.Register()
  ...
}

[ansel1]

I would error on the side of explicit registration. every time I give in to the temptation to have any kind of external effect in an init function, for the sake of elegance or ergonomics, I get burned. almost every time.

contrived example: your package registers two encoders. someone else's packages registers an encoder with the same name as one of yours. if this is all done in init functions, the registration order will be indeterminate.

[prashantv]

+1 on letting third-party encodings be registered with the inbuilt config. From zap's perspective, it will expose a RegisterEncoder and whether that's done at init time or as an explicit function is really up to the third-party library, the API stays the same.

We can recommend explicit registrations, but we shouldn't (and can't easily) enforce this.

[bufdev]

I'll do a PR for this after #307 (sorry I'm pinging so much @akshayjshah :) ).

[akshayjshah]

Yep, no worries. I'm (of course) also in favor of third-party packages using some explicit registration function, but I suspect that in practice we'll end up with import-time side effects. C'est la vie.

PRs welcome.

[akshayjshah]

Landed.

[lomik]

There is another problem.

Encoder.EncodeEntry should return Buffer object. And this Buffer here returns to the pool. But custom Encoder can't use bufferpool because it in go.uber.org/zap/internal/bufferpool package.

[jsternberg]

👍 to that. While trying to make my custom encoder, I ended up just copying the internal package locally so I could use it. It would be nice if bufferpool was made public since it seems to be such an integral part of the logging system.

[akshayjshah]

Making the buffer pool public isn't a good idea, since it allows third-party packages to easily introduce memory corruption - anyone who retains a reference to a buffer will pollute the buffer pool for everyone (and introduce data races).

There's also little point in copying the pooling code, since zap can't return the buffer to your pool - it'll always be empty, so you'll always go through New.

This isn't ideal, but it only introduces a single allocation in third-party code. I'm open to other ideas, as long as they don't introduce the possibility of data races and memory corruption.

[akshayjshah]

Thought about this a bit and came up with a solution that should make everyone happy: zap keeps its buffer pool private and safe, but other encoders can still get the benefits of pooling.

See #376.

[lomik]

The perfect solution. Thank you