Add support for Loggers.

[ShaileshSurya]

[ZachPlunkett]

Hello Shailesh,

I would be down to put some time into this. I updated the README.md and added a little example but I don't have any permissions right now. I worked with go over the summer a good bit at my internship but I want to get better. I figure a wrapper around the net package would be good practice.

What is the best way to move forward with this?

[ShaileshSurya]

Hi @ZachPlunkett thank you for the PR with README.MD. It was much needed.

This ticket needs some investigation. I did not get a time to go around this but here is a rough idea's.

1. As this is a library, we should adhere to what the user is using for logging. Logs from library are mostly considered unnecessary. So we should log only in Debug Mode.
2. We can use the logger function provided by user.
3. There are many loggers in use, Logrus, Logger and other. So we should come up with common interface.

or we can take look at how the other libraries have solved this.

for example:- https://github.com/go-gorm/gorm

There is a scope for more innovative ways to handle this. Let me know your thoughts

[ZachPlunkett]

I am somewhat familiar with Uber-go/Zap but I worked with logs primarily from a data ingestion point of view. In this case I am confused I guess on what should be logged in the first place.

Are we wanting to log the all of the information that the user adds to the request to the debug log? Like give them back the url, headers, etc....

What about information regrading timeouts? If the server closes the connection does that go to the log or is it returned as an error or both?

[ShaileshSurya]

@ZachPlunkett , As a library we should log only in the Debug Mode.

What to Log? As this is a HTTPRequestClient we should log the Curl Command. We already have code https://github.com/ShaileshSurya/hammer/blob/master/http2curl.go to convert the curl command from the Request.

What if server throws error? If the server closes the connection server will send out the Response. We will pass on the error to the user.

[ZachPlunkett]

@ShaileshSurya, okay so I think I have a better understanding.

This debug logger will not have to do much then, primarily it will use func GetCurlCommand(req *http.Request) to show in a human readable format what the Request being sent was.

In the future with the addition of a response builder I see a similar use case for the logger. It wouldn't really need to show issues with the connection since that would be given to the user as an error.

As for how to go about implementing the logger using one given by the user makes sense. It seems that gorm wants anthing that implements Printf(string, interface{}...) assuming most loggers implement that function.

• log implements Printf(string, interface{})
• logrus implements Printf(string, interface{})
• glog does not

So implementing a logger similar to gorm would force the user of the library to deal with the problem if their current logger does not implement Printf(string, interface{}...).

I know you mentioned a more innovative way of handling this but I didn't think up anything differently myself.

[ShaileshSurya]

@ZachPlunkett Thats a real good finding. Here are some of the problems that we have in hand.

1. Typical production environment runs on the INFO level, whereas developer environment runs on the DEBUG level. The level is changed like a switch, at only one place and it applies to all the system. Hence we need the logger instance from the user and change the libraries logging behavior accordingly.
2. In addition to the above mentioned point drawback for gorm, the user creates the client once and then use that singleton instance for all the DB operations. Hence, hooking the logger only once. In our case creating a HammerClient is a trivial thing and user might create it multiple times. so hooking the logger each time might not be a good experience.

Therefore, I said there is need of handling this problem in more innovative way. I also don't have anything as such on my mind but this is a real exciting problem we can get our heads round on.

[ZachPlunkett]

@ShaileshSurya I see now that your hammer struct currently has a LogFunc func(msg string. I am assuming you previously were intending to use this function in the Execute() possibly like:

func (r *Hammer) Execute(req *Request) (*http.Response, error) {
        if r.debugMode {
                curlCommand, err:= GetCurlCommand(req)
                if err != nil { return nil, error }
                r.LogFunc(strings.Join(curlCommand, ""))
        }
	httpClient := r.getHTTPClient()
	return req.doRequest(httpClient)
}

As you mentioned this does not satisfy point 1 above and still has the limitations in point 2 above.

Global Logger

Having the LogFunc given by the user be assigned to a global global variable would make setting up many instances hammer be easy using the same logger but that eliminates the ability to use multiple loggers for separate systems. That seems horrible in my head to be honest just the only thing I can think of so far.

Global Logger with Context

Would it be possible to associate a global logger with a context? (I am still new-ish to go plz don't kill me if that is the dumbest thing you every read lol) That would allow multiple instances of hammer to be setup with specific loggers attached to the context given to the hammer instance. I feel this would be hard to keep track of and/or impossible.

Attach logger to a request

In a very different direction for the user to implement some interface or struct that could then be added into the chain of functions on each request. Then if the global debugMode is set is will log, otherwise it will not. Again I feel this is a poor idea

request, err := hammer.RequestBuilder().
		Get().
                WithDebug(logFunc).
		WithURL("http://echo.jsontest.com/Greeting/hello/place/world").
		WithContext(context.Background()).
		WithHeaders("Accept", "application/json").
		Build()

[ShaileshSurya]

@ZachPlunkett The suggestion made by you are great. We can discuss all of those in details.

Can you please have look at this article. The author suggests a logger like the gorm. Do have a look and do let the forum know if you think this might help us with our scenario in had.