Support http client middlewareing

[michurin]

Description

It would be great to be able to wrap http.Client for logging, having metrics, timings, alerting etc.

Solution

Just use interface instead of concrete type.

Detailed example

For instance, I would like to be able to do something like that:

type httpClientIf interface {
        Do(req *http.Request) (*http.Response, error)
}

type debugClient struct {
        next httpClientIf
}

func (dc *debugClient) Do(req *http.Request) (*http.Response, error) {
        t := time.Now()
        r, err := dc.next.Do(req)
        fmt.Printf("%s %s -> %v -> %d\n", r.Request.Method, r.Request.URL.String(), time.Since(t), r.StatusCode)
        return r, err
}

...

        client := openai.NewClientWithConfig(openai.ClientConfig{
                HTTPClient: &debugClient{next: http.DefaultClient}, // <--- wrapping, thanks to interfaces
        })

To see loglines like this:

POST http://127.0.0.1:11434/v1/chat/completions -> 12.973492155s -> 200

[michurin]

By the way, it would be nice for mocking as well

[eiixy]

This implementation might be easier to use.

// Handler defines the handler invoked by Middleware.
type Handler func(req *http.Request, v Response) error

// Middleware is HTTP transport middleware.
type Middleware func(Handler) Handler

// Client is OpenAI GPT-3 API client.
type Client struct {
	config            ClientConfig
	middleware        Middleware
	requestBuilder    utils.RequestBuilder
	createFormBuilder func(io.Writer) utils.FormBuilder
}


func (c *Client) sendRequest(req *http.Request, v Response) error {
	next := func(req *http.Request, v Response) error {
		req.Header.Set("Accept", "application/json")

		// Check whether Content-Type is already set, Upload Files API requires
		// Content-Type == multipart/form-data
		contentType := req.Header.Get("Content-Type")
		if contentType == "" {
			req.Header.Set("Content-Type", "application/json")
		}

		res, err := c.config.HTTPClient.Do(req)
		if err != nil {
			return err
		}

		defer res.Body.Close()

		if v != nil {
			v.SetHeader(res.Header)
		}

		if isFailureStatusCode(res) {
			return c.handleErrorResp(res)
		}

		return decodeResponse(res.Body, v)
	}
	if c.middleware != nil {
		return c.middleware(next)(req, v)
	}
	return next(req, v)
}

&Client{
	config: config,
	middleware: func(next Handler) Handler {
		return func(req *http.Request, v Response) error {
			t := time.Now()
			err := next(req, v)
			log.Printf("%s %s -> %v -> %d\n", req.Method, req.URL.String(), time.Since(t), v)
			return err
		}
	},
	requestBuilder: utils.NewRequestBuilder(),
	createFormBuilder: func(body io.Writer) utils.FormBuilder {
		return utils.NewFormBuilder(body)
	},
}

[michurin]

I'm not sure your solution is easier. It less obvious, it leaves room for uncertainty: maybe middleware has to be a member of ClientConfig? may be middleware has to be a slice []Middleware? if it is slice, what order it would be applied? Too much uncertainty. I vote for just simple and straight forward interface-based approach

[eiixy]

or something like this.

config := openai.DefaultConfig(test.GetTestToken(), func(next openai.HandlerFunc) openai.HandlerFunc {
	return func(r *http.Request) (*http.Response, error) {
		t := time.Now()
		response, err := next(r)
		log.Printf("%s %s -> %v -> %d\n", r.Method, r.URL.String(), time.Since(t), response.StatusCode)
		if err != nil {
			return nil, err
		}
		return response, nil
	}
})

type HandlerFunc func(r *http.Request) (*http.Response, error)

func (h HandlerFunc) RoundTrip(r *http.Request) (*http.Response, error) {
	return h(r)
}

type Middleware func(next HandlerFunc) HandlerFunc

func DefaultConfig(authToken string, middleware ...Middleware) ClientConfig {
	var transport = http.DefaultTransport
	for _, m := range middleware {
		transport = m(transport.RoundTrip)
	}
	return ClientConfig{
		authToken:        authToken,
		BaseURL:          openaiAPIURLv1,
		APIType:          APITypeOpenAI,
		AssistantVersion: defaultAssistantVersion,
		OrgID:            "",

		HTTPClient: &http.Client{Transport: transport},

		EmptyMessagesLimit: defaultEmptyMessagesLimit,
	}
}

[michurin]

You are right, we are free to use http.Client.Transport right now. However, Transport is not the same: transport considering all individual HTTP request. Sometimes it is what we wont, sometimes it is not.

Let me show you what I mean. Consider this:

type TransportFunc func(r *http.Request) (*http.Response, error)

func (h TransportFunc) RoundTrip(r *http.Request) (*http.Response, error) {
	return h(r)
}

func main() {
	c := &http.Client{
		Transport: TransportFunc(func(r *http.Request) (*http.Response, error) {
			t := time.Now()
			response, err := http.DefaultTransport.RoundTrip(r)
			log.Printf("%s %s -> %v -> %d\n", r.Method, r.URL.String(), time.Since(t), response.StatusCode)
			return response, err
		}),
	}
	r, err := c.Get("http://google.com")
	if err != nil {
		panic(err)
	}
	fmt.Println("HTTP status:", r.Status)
}

The result will be:

2024/08/23 07:20:54 GET http://google.com -> 59.157971ms -> 301
2024/08/23 07:20:54 GET http://www.google.com/ -> 82.410694ms -> 200
HTTP status: 200 OK

More over, Transport could be useful for logging, it could be considered for metrics/monitoring/alerting approach... however, it would be very strange to use it for things like mocking.

My point, it is good, that we can use custom transport. However, it would be great to be able to use custom client. By the way, http.Client.Transport is interface. It's common approach. It would be nice to do the same trick with openai.ClientConfig.HTTPClient for the same reasons: unlock mocking, wrapping etc.

[sashabaranov]

@michurin thank you for the PR! The need for mocking makes total sense and I'm a bit surprised that http library doesn't offer us such an interface.

My only ask would be to please make it non-anonymous — we can either call it HttpClient or HttpDoer (similar to Writer/Reader).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (774fc9d) to head (0362845).
Report is 43 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
+ Coverage   98.46%   99.01%   +0.55%     
==========================================
  Files          24       26       +2     
  Lines        1364     1418      +54     
==========================================
+ Hits         1343     1404      +61     
+ Misses         15        8       -7     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michurin]

@sashabaranov I totally agree. PR's updated