go-playground / log

:green_book: Simple, configurable and scalable Structured Logging for Go.
MIT License
293 stars 21 forks source link

New Redis & HTTP handlers #3

Closed hartfordfive closed 6 years ago

hartfordfive commented 8 years ago

@joeybloggs I've added these two new handlers. Please note this is an initial version and I'd like to improve them both in the near future by adding a feature that allows multiple items to be buffered and sent in a MULTI command to Redis, and allow the option for multiple events to be buffered and sent at once for the HTTP handler.

I haven't added any tests for the HTTP handler as I'm not completely certain how to test for that yet. Would it be reasonable to have spawn a local HTTP server listening for events for this test?

I also plan on adding a file handler, which allows you to log events to a specified file. Let me know if this code is reasonable, and let me know if you have any suggestions for improvements/modifications!

deankarn commented 8 years ago

Thanks @hartfordfive!!

hoping to have a look later tonight.

as for testing the http handler, I think it's totally reasonable to spawn a local http server instance using go's httptest package https://golang.org/pkg/net/http/httptest/

deankarn commented 8 years ago

Wow you did a ton of work! @hartfordfive

there are a few things I would change, but all very minor. like:

    if bufferSize >= 1 {
        h.buffer = bufferSize
    }

Question

Because handlers work off of a channel, many workers could be created to handle logs; do you think an option should be added to specify a number of workers?

I will help further as the week progresses

hartfordfive commented 8 years ago

@joeybloggs I'll make all those small suggested modifications. As for the number of workers, I totally agree and I'll add that also. One other potential modification would be to add the "runtime. GOMAXPROCS()" function to your log.go file to allow the use for one or more CPU cores? This would definitely help with message throughput when it comes to the HTTP or Redis handler.

deankarn commented 8 years ago

Thanks @hartfordfive

As for runtime.GOMAXPROCS() definitly shouldn't play around with that setting in a library. As of Go 1.5 it is by default set to the number CPU Cores in the system.

Morover this is a process wide setting and would affect the entire running application.

hartfordfive commented 8 years ago

@joeybloggs Good point on that. As for the tests, I'll have those finished hopefully within the next few days.

deankarn commented 8 years ago

No rush and thanks again

hartfordfive commented 8 years ago

@joeybloggs Regarding the Redis handler, that test suite requires a local instance considering there is no equivalent testing package as with httptest. Otherwise, I can't see another concrete method of testing it. What's your opinion on that?

deankarn commented 8 years ago

I use Semaphore CI for my testing which has a redis database running on the default port, as long as the tests point to 127.0.0.1 and default redis port it should work

deankarn commented 8 years ago

Hey @hartfordfive , sorry I've been a bit busy getting the library to v2.0 but just thought I'd let you know and the http handler made it into the release!

I will be looking into the redis handler soon enough, I'm just trying to find a way to do the following:

1) Reuse the redis connection (tcp) and reconnect when disconnected, for efficiency. 2) The ability to use TLS redis connection which I don't see an easy way with the redis driver used.