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

New Redis & HTTP handlers #3

Closed
wants to merge 6 commits into from

Conversation

hartfordfive
Copy link

@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
Copy link
Contributor

deankarn commented May 2, 2016

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
Copy link
Contributor

deankarn commented May 3, 2016

Wow you did a ton of work! @hartfordfive

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

  • in examples use log.Debug(...) and log.Info(...) instead of manually creating Entry and calling HandleEntry(...)
  • (this ones my fault) changed from e.WG.Done() to e.Consumed() hiding the backend sync mechanism and allowing for changes without breaking compatibility.
  • perhaps instead of calling fmt.Println(...) on error maybe just call log.Error() that way it will get logged somewhere, like another registered logger ( console logger perhaps )
  • bufferSize is a uint and so will always be >= 0 so can just directly assign to new struct.
    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
Copy link
Author

@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
Copy link
Contributor

deankarn commented May 3, 2016

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
Copy link
Author

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

@deankarn
Copy link
Contributor

deankarn commented May 3, 2016

No rush and thanks again

@hartfordfive
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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.

@deankarn deankarn closed this Feb 12, 2018
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.

2 participants