Decoupled logrus and added a logger interface

[owais]

Fixed #10

- `producer.Config` now takes a producer.Logger interface instead of
a Logrus instance.
- If not logger is provided to config, producer uses a standard library
logger by default (`producer.StandardLogger`)
- `loggers` sub package ships implementations for zap and logrus.

[owais]

@a8m Added a logger interface and a couple of implementations (logrus and stdlib). Verified that both work. I think the lib should only log info and error messages and not try to deal with all possible "levels".

I'm not sure if the lib should include implementations for popular loggers such as logurs, zap, etc as it would pull in those dependencies whether the users need them or not. Probably they should be published as independent packages. Thoughts?

[a8m]

Hey @owais, thanks so much for your contribution. I've added a few comments.
Another thing, imo it makes more sense to leave all logging code under the root (producer) package.
As a user of this package, I prefer to not import a generic/common package (e.g: logging) that will conflict with other packages.

[a8m]

I would rename this struct into something like LogEntry, since it's a bit confusing that it's under producer namespace.

[owais]

Sure. I moved it from the loggers package to top level but didn't rename,.

[a8m]

Suggested change

	Error(msg string, err error, values ...Value)
	Info(args ...interface{})
	Error(args ...interface{})

What do you say about changing the interface signature to something like this? this will save the Value entry and the logrus/zap implementations.
You can even make it a field-logger by treat the args param as a list of key-value pairs.

[owais]

I personally don't like it as almost all loggers at least expect a string so taking interface{} where you know you need a string doesn't sound too good to me. Also, I think having a struct to define a key value pair is better than an even number of args and then grouping them inside the function. I think it's cleaner for both the API and the internal implementation. I don't see much value in doing it but you are maintaining the code so I can do what makes your workflow better.

[owais]

As a user of this package, I prefer to not import a generic/common package (e.g: logging) that will conflict with other packages.

The reason I put it inside a sub-package was to avoid making zap and logrus dependencies (in near future). I was thinking that producer/loggers can be a separate go package hosted in the same repo. Depending on kinesis-producer would not automatically install loggers meaning producer itself would not depend on logrus, zap and any other implementations people might contribute. I don't know how it would work exactly but I hope having go.mod at both the root and inside loggers should support this. (plan to add support for go.mod as well in the coming weeks).

[owais]

That said, I can move it to the root package for now and we can move it back or out of the repo if needed/wanted in future.

[owais]

@a8m Incorporated some suggestions. Let me know your decision on root vs sub-package for logger implementations. I personally think it should actually be like the following but am open to moving everything to root'

root/
  - library_code 
  - loggers/
      - interface_and_stock_logger.go
      - logrus/
         - logrus.go
      - zap/
         - zap.go

[a8m]

The reason I put it inside a sub-package was to avoid making zap and logrus dependencies (in near future).

I agree with this, and that's why I suggested creating an interface that common log packages support by default, and we should add only the default implementation, which is the std-log.

About directories structure, my only problem with this is that I don't want to create a package with common name, or to conflict with logrus or zap, so people will need to alias it to something else.

[owais]

OK, makes sense. In that case, we could use a less common but somewhat ugly names. Something like:

root/
  - library_code 
  - interface_and_stock_logger.go
  - go.mod (eventually)
  - loggers/
      - kpLogrus/
         - logrus.go
         - go.mod (eventually)
      - kpZap/
         - zap.go
         - go.mod (eventually)

[a8m]

Go's packages are usually lowercased with no underscore or mixedCaps (ref). But yeah, I agree, pkzap looks a bit hacky, but I don't have a better suggestion...

[a8m]

Thanks for the contribution and your patient @owais.