Improvements for request/response dumping

[dancavallaro]

I'm using Gofish in a project and I'm working on some test automation using recorded+replayed Redfish interactions. I configure my APIClient with a dumpWriter to log requests/responses to a file, then later do some automated post-processing on the file to generate structured data for my tests.

I'm able to get it working pretty well, but it would be easier to do this kind of thing if APIClient allowed more control over how requests and responses are dumped, and I was wondering if you'd potentially be open to a PR after discussing the approach.

The main thing that would be the biggest improvement IMO would be the ability to group requests and corresponding responses to be dumped together, e.g. in the same log file entry. The current approach of dumping them separately means that responses may not immediately follow their request, if the client is being used concurrently. Secondarily, it could be useful to have more control over how the requests/responses are dumped, i.e. with direct access to the http.Request/http.Response.

First, am I missing any alternative approaches that might give me what I'm looking for without any changes to Gofish? It occurs to me that I could probably do this by configuring the http.Client myself with some kind of logging RoundTripper, without needing any support from Gofish, but that would make it a lot simpler.

Would you be open to extending the dumpWriter support with something like an optional Dumper that you could give to the ClientConfig? I'm imagining something like this:

type Dumper interface {
  Dump(req *http.Request, resp *http.Response) error
}

Then users who want the flexibility can use a custom Dumper and log the requests/responses however they want, but for backwards compatibility there could be a default dumper that maintains the current behavior using a dumpWriter, if provided.

Any thoughts? I'd be happy to work on a PR if you're willing to consider it, but my feelings won't be hurt if you tell me to go away and use a custom HTTP client.

[stmcginnis]

Hey @dancavallaro, thanks for bringing this up. Yeah, any PR would be welcome. I see how that could be useful. When this request/response dumping was added, it was mainly just a debugging aid. It looks like you have a valid use case for improving that facility, so feel free to propose something that would meet your needs better. Thanks!

[dancavallaro]

Got a little sidetracked but I did manage to spend some time playing with this again, and realized that the idiomatic way of doing this kind of thing in Go seems to be using the RoundTripper interface, which lets you build "interceptor"/"middleware" functionality like this.

So that obviates the need for the new Dumper interface I was proposing, and the only remaining issue is how to inject my logging interceptor into Gofish's HTTP client.

I saw that the Gofish code is mostly using the default http.DefaultTransport, with a couple customizations based on client config. What I really wanted to do was find a way to just inject my RoundTripper into the existing Gofish client init code, but I couldn't find a way to do that so I essentially just copy and pasted Gofish's initialization of the http.Transport struct, and then injected my RoundTripper in there (which itself is a fairly simple logging struct that wraps the default Transport).

So long story short I'm unblocked, and it'd be nice to have a way to more directly inject my RoundTripper into the existing Transport, but it's only a minor inconvenience. If you were to add support for this in the Gofish client, one idea that comes to mind would be to add a new optional func(*http.Transport) *http.Transport parameter to ClientConfig (Interceptor? WrapTransport?). If provided Gofish would call this function with the custom Transport that it creates, and use the returned Transport to finally construct the http.Client.

Here's a simplified version of my logging Transport:

type recordingTransport struct {
    transport *http.Transport
}

func (r recordingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
    resp, err := r.transport.RoundTrip(req)
    // Log the request/response, do whatever you want, then return the response and error.
}

Currenty, I'm injecting it into an HTTP client by doing this:

func createRedfishClient() *gofish.APIClient {
    defaultTransport := http.DefaultTransport.(*http.Transport)
    baseTransport := &http.Transport{
        Proxy:                 defaultTransport.Proxy,
        DialContext:           defaultTransport.DialContext,
        MaxIdleConns:          defaultTransport.MaxIdleConns,
        IdleConnTimeout:       defaultTransport.IdleConnTimeout,
        ExpectContinueTimeout: defaultTransport.ExpectContinueTimeout,
        TLSHandshakeTimeout:   time.Duration(10) * time.Second,
        TLSClientConfig: &tls.Config{
            InsecureSkipVerify: true,
        },
    }
    transport := recordingTransport{baseTransport}
    httpClient := &http.Client{Transport: transport}

    config := gofish.ClientConfig{
        Endpoint:   fmt.Sprintf("https://%s", host),
        Username:   username,
        Password:   password,
        HTTPClient: httpClient,
    }
    redfish, err := gofish.Connect(config)
    ...
}

With something like a WrapTransport func(*http.Transport) *http.Transport parameter, I could do something like this instead of reconstructing the transport:

func createRedfishClient() *gofish.APIClient {
    config := gofish.ClientConfig{
        Endpoint:   fmt.Sprintf("https://%s", host),
        Username:   username,
        Password:   password,
        WrapTransport: func(t *http.RoundTripper) *http.RoundTripper {
            return recordingTransport{baseTransport}
        },
        HTTPClient: httpClient,
    }
    redfish, err := gofish.Connect(config)
    ...
}

I'm happy to contribute something like that if you're open to it, but I also understand if you think it's not general enough to be useful and would rather just close this. Like I said, I'm unblocked now, and either way thanks for your help!