grafana / k6

A modern load testing tool, using Go and JavaScript - https://k6.io
GNU Affero General Public License v3.0
25.94k stars 1.26k forks source link

--http-debug incorrectly reports http protocol #986

Open mstoykov opened 5 years ago

mstoykov commented 5 years ago

--http-debug uses httputil.DumpRequestOut which as mention in the httputil.DumpRequest dumps http2 requests as HTTP/1.1 this is ... at best confusing.

We should probably dump the request after the real connection, although my quick test showed this did not help :)

mstoykov commented 11 months ago

We should probably dump the request after the real connection, although my quick test showed this did not help :)

Tried again, and you also need to change to DumpRequest from DumpRequestOut as the later does in practice "sent" the request locally to see what the stdlib will do so :+1: on this.

Unfortunately this also masks anything we set from the response.

diff --git a/lib/netext/httpext/httpdebug_transport.go b/lib/netext/httpext/httpdebug_transport.go
index f242dfcd..c82632e3 100644
--- a/lib/netext/httpext/httpdebug_transport.go
+++ b/lib/netext/httpext/httpdebug_transport.go
@@ -23,14 +23,18 @@ type httpDebugTransport struct {
 //   - https://github.com/k6io/k6/issues/774
 func (t httpDebugTransport) RoundTrip(req *http.Request) (*http.Response, error) {
        id, _ := uuid.NewV4()
-       t.debugRequest(req, id.String())
        resp, err := t.originalTransport.RoundTrip(req)
+       if resp != nil {
+               req.ProtoMajor = resp.ProtoMajor
+               req.ProtoMinor = resp.ProtoMinor
+       }
+       t.debugRequest(req, id.String())
        t.debugResponse(resp, id.String())
        return resp, err
 }

 func (t httpDebugTransport) debugRequest(req *http.Request, requestID string) {
-       dump, err := httputil.DumpRequestOut(req, t.httpDebugOption == "full")
+       dump, err := httputil.DumpRequest(req, t.httpDebugOption == "full")
        if err != nil {
                t.logger.Error(err)
        }

But this now will not showcase the remaining parts the transport will add if not set - so we likely need to set them ...

This might also help #1042, although this might be a bit of breaking change.