Expose HTTP request information in response #447

cstyan · 2018-01-11T07:01:59Z

Fixes #440

Adds HTTPRequest struct, includes the request as a field in the response, and finally a test for whether or not the request information is properly populated in a response.

cc: @robingustafsson @marklagendijk

codecov-io · 2018-01-11T07:10:16Z

Codecov Report

Merging #447 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #447      +/-   ##
=========================================
+ Coverage   62.44%   62.5%   +0.05%     
=========================================
  Files          93      93              
  Lines        6679    6689      +10     
=========================================
+ Hits         4171    4181      +10     
  Misses       2264    2264              
  Partials      244     244

Impacted Files	Coverage Δ
js/modules/k6/http/response.go	`67.36% <ø> (ø)`	⬆️
js/modules/k6/http/http_request.go	`88.81% <100%> (+0.31%)`	⬆️
js/modules/k6/http/http.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f882340...607866b. Read the comment docs.

marklagendijk · 2018-01-11T09:50:51Z

Well done! LGTM!

robingustafsson

Great, thanks @cstyan! We have a few things that needs adjusting.

robingustafsson · 2018-01-11T10:12:51Z

js/modules/k6/http/http_request.go

@@ -109,9 +117,14 @@ func (h *HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.Stat
 		URL:    url.URL,
 		Header: make(http.Header),
 	}
+	HTTPReq := &HTTPRequest{


Could we change the name to respReq or something more descriptive (and start with lowercase letter to mainntain similar var name style)?

robingustafsson · 2018-01-11T10:18:29Z

js/modules/k6/http/http_request.go

@@ -109,9 +117,14 @@ func (h *HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.Stat
 		URL:    url.URL,
 		Header: make(http.Header),
 	}
+	HTTPReq := &HTTPRequest{
+		Method: req.Method,
+		URL:    req.URL,


Hm, I think this should be req.URL.URLString to be consistent with res.url (which is just a string). We should expose URL parsing/joining/composing functionality in a specific module though.

robingustafsson · 2018-01-11T10:20:42Z

js/modules/k6/http/response.go

@@ -62,6 +62,8 @@ type HTTPResponse struct {
 	OCSP           OCSP `js:"ocsp"`
 	Error          string



You can remove the newline here.

robingustafsson · 2018-01-11T10:31:26Z

js/modules/k6/http/http_request.go

+	URL     *neturl.URL
+	Headers map[string][]string
+	Body    io.Closer
+	Cookies map[string]*HTTPRequestCookie


The Cookie header is allowed to contain multiple cookies with the same name [1] so Cookies here needs to be a map[string][]*HTTPRequestCookie, or map[string][]string would be fine as well in my opinion.

[1] - https://tools.ietf.org/html/rfc6265#section-4.2

Here I've just used the same types in the map as are being used for the cookies being put into the request struct: https://github.com/loadimpact/k6/blob/master/js/modules/k6/http/http_request.go#L139

Does that map need to change as well?

No, that map only stores the user specified per-request cookies (from JS, like the first example here: https://docs.k6.io/docs/cookies#section-setting-simple-cookies).

But to get all cookies that are sent with the request (we is what we want to expose through res.request.cookies) we also need to look at what's in the cookiejar (from any previous interactions with the server). And that's when the situation can arise where we end up with multiple cookies with the same name.

Merging the per-request cookies set by the user in JS land with any cookies already present in the cookiejar is what https://github.com/loadimpact/k6/blob/master/js/modules/k6/http/http.go#L130 does.

robingustafsson · 2018-01-11T10:36:51Z

js/modules/k6/http/http_request.go

@@ -235,7 +248,10 @@ func (h *HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.Stat
 		}
 	}

-	resp := &HTTPResponse{ctx: ctx, URL: url.URLString}
+	HTTPReq.Headers = req.Header
+	HTTPReq.Cookies = reqCookies


The cookies sent with the request could also be retrieved from the active cookiejar so we need similar logic to what is in https://github.com/loadimpact/k6/blob/master/js/modules/k6/http/http.go#L130 (called a right above this) to determine the actual cookies sent. I guess the easiest would be to have setRequestCookies() return the determined set of cookies to send (jarCookies).

Not sure I understand completely. I think you're saying we want to add getRequestCookies() to get the cookies that would have actually have been put into the http.Request struct?

cstyan · 2018-01-16T07:55:44Z

@robingustafsson I've changed the cookies to []*http.Cookie, which is the same type returned by http.Request.Cookies(). I didn't see a nice way without some more refactoring to have []*http.Cookie in the actual request and map[string][]*HTTPRequestCookie or map[string][]string in the request struct that is returned via the response.

marklagendijk · 2018-01-16T09:58:10Z

@cstyan can't you just do something like this?

cookies := req.Cookies()
respReq.Cookies := make(map[string][]string)
for _, cookie := range cookies {
	if array, exists := respReq.Cookies[cookie.Name]; exists{
		respReq.Cookies[cookie.Name] = append(array, cookie.Value)
	} else {
		respReq.Cookies[cookie.Name] = []string { cookie.Value }
	}
	
}

cstyan · 2018-01-17T06:53:14Z

@marklagendijk Yep, but I didn't want to just duplicate code from setRequestCookies https://github.com/loadimpact/k6/blob/master/js/modules/k6/http/http.go#L130

I was over thinking things yesterday, the refactor is simple.

marklagendijk · 2018-01-17T10:18:54Z

js/modules/k6/http/http_request.go

@@ -25,6 +25,7 @@ import (
 	"compress/gzip"
 	"compress/zlib"
 	"context"
+	// "fmt"


Can be removed.

marklagendijk · 2018-01-17T10:20:44Z

js/modules/k6/http/http_request_test.go

+
+			if (res.status != 200) { throw new Error("wrong status: " + res.status); }
+			if (res.request["method"] !== "GET") { throw new Error("http request method was not \"GET\": " + JSON.stringify(res.request)) }
+			if (res.request["body"] != null) { throw new Error("http request body was not null: " + JSON.stringify(res.request)) }


Missing test case for when body should be set. When testing this myself it seems that request.body does not work:

panic: reflect.Value.Addr of unaddressable value [recovered]--------] 0 / 1 panic: Panic at 86: reflect.Value.Addr of unaddressable value [recovered] panic: Panic at 86: reflect.Value.Addr of unaddressable value goroutine 37 [running]: github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1.1(0xc422d2bd00) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1407 +0xa2 panic(0xb53720, 0xc4201360d0) /home/markl/.gvm/gos/go1.9.2/src/runtime/panic.go:491 +0x283 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).try.func1(0xc42203b450, 0x0, 0xc422d2bc08, 0x0, 0x0, 0x0, 0xc422d2bc60) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:364 +0x4b4 panic(0xb2b620, 0xcdc1d0) /home/markl/.gvm/gos/go1.9.2/src/runtime/panic.go:491 +0x283 reflect.Value.Addr(0xb70f00, 0xc420609a30, 0x94, 0xc423036280, 0x1, 0x1) /home/markl/.gvm/gos/go1.9.2/src/reflect/value.go:239 +0x82 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*objectGoReflect)._getField(0xc42226e510, 0xc423036268, 0x6, 0x411eb7, 0xc4201360a0, 0x10) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/object_goreflect.go:93 +0xf8 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*objectGoReflect)._get(0xc42226e510, 0xc423036268, 0x6, 0x10, 0xc4201360a0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/object_goreflect.go:111 +0xfc github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*objectGoReflect).getStr(0xc42226e510, 0xc423036268, 0x6, 0xc423055e20, 0x20) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/object_goreflect.go:124 +0x43 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*objectGoReflect).get(0xc42226e510, 0x1131f00, 0xc420136030, 0x18, 0xc423055e20) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/object_goreflect.go:86 +0x3f github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).str(0xc421b8c820, 0x1131f00, 0xc420136030, 0xc4230551c0, 0x0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:283 +0x6e github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).jo(0xc421b8c820, 0xc4230551c0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:458 +0x1f1 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).str(0xc421b8c820, 0x1131f00, 0xc4203b9d20, 0xc423054900, 0x0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:370 +0x3be github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).jo(0xc421b8c820, 0xc423054900) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:458 +0x1f1 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).str(0xc421b8c820, 0x1131f00, 0x12bfc00, 0xc4230556c0, 0x12bfc00) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:370 +0x3be github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*_builtinJSON_stringifyContext).do(0xc421b8c820, 0x1131d20, 0xc423054900, 0xc423036334) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:279 +0x1de github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*Runtime).builtinJSON_stringify(0xc422870480, 0x1131d20, 0xc42187ab80, 0xc423121e60, 0x3, 0x3, 0xc423055640, 0xc420c18500) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:270 +0x7d4 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*Runtime).(github.com/loadimpact/k6/vendor/github.com/dop251/goja.builtinJSON_stringify)-fm(0x1131d20, 0xc42187ab80, 0xc423121e60, 0x3, 0x3, 0x3, 0x4126f0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_json.go:517 +0x51 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*Runtime).functionproto_apply(0xc422870480, 0x1131d20, 0xc42187abe0, 0xc421c9d450, 0x2, 0x39, 0xc423036310, 0xc422d2b8c0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_function.go:59 +0x143 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*Runtime).(github.com/loadimpact/k6/vendor/github.com/dop251/goja.functionproto_apply)-fm(0x1131d20, 0xc42187abe0, 0xc421c9d450, 0x2, 0x39, 0x5, 0x1) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_function.go:159 +0x51 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm)._nativeCall(0xc42203b450, 0xc423004370, 0x2) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:1826 +0x2ec github.com/loadimpact/k6/vendor/github.com/dop251/goja.call.exec(0x2, 0xc42203b450) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:1810 +0x6b3 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*call).exec(0xc42283c0ec, 0xc42203b450) <autogenerated>:1 +0x44 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).run(0xc42203b450) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:288 +0x51 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*funcObject).Call(0xc420ca1500, 0x1132360, 0x12e2100, 0x0, 0x0, 0x0, 0x456c17, 0xc42002c600) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/func.go:130 +0x3c8 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*funcObject).Call-fm(0x1132360, 0x12e2100, 0x0, 0x0, 0x0, 0x42a5c9, 0xc4212c0030) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/builtin_function.go:120 +0x51 github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1.2() /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1412 +0xb6 github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).try(0xc42203b450, 0xc420049c78, 0x0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:370 +0x119 github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1(0x1132360, 0x12e2100, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1411 +0x12e github.com/loadimpact/k6/js.(*VU).RunOnce(0xc4216ac930, 0x112d240, 0xc4216d00c0, 0x0, 0xc4222d4bb0, 0xc420b979f0, 0xc420b95bb0, 0xc420b979e0) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/js/runner.go:260 +0x4c1 github.com/loadimpact/k6/core/local.(*vuHandle).run(0xc421517180, 0xc42007aaa0, 0xc42310e1e0, 0xc42310e180) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/core/local/local.go:64 +0x1cc github.com/loadimpact/k6/core/local.(*Executor).scale.func1(0xc421517180, 0xc423177c20, 0xc42310e1e0, 0xc42310e180) /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/core/local/local.go:315 +0x4d created by github.com/loadimpact/k6/core/local.(*Executor).scale /home/markl/.gvm/pkgsets/go1.9.2/global/src/github.com/loadimpact/k6/core/local/local.go:314 +0x35f

oof, okay I'll look into this!

…alue doesn't seem to be passed to JS code properly

cstyan · 2018-01-28T01:03:23Z

I think this is good to go

robingustafsson · 2018-01-29T11:41:13Z

@cstyan Yes, looks good. The only thing I'm a bit concerned about it is the added memory copy resulting from respReq.Body = bodyBuf.String(). In the upcoming Go 1.10 release there's a new strings.Builder that we could perhaps make use of later (golang/go@37b0569) to avoid the memory copy, but we'll have to look closer at this for #370 as well where respReq.Body = bodyBuf.String() will probably change to respReq.Body = bodyBuf.Bytes(), that would afaict also avoid a memory copy.

I'll merge this PR and then evaluate how the memory copy can be avoided as part of #370 (as it's mostly going to be costly when doing larger uploads, binary data like images, videos etc.).

cstyan changed the title ~~Callum 440~~ Expose HTTP request information in response Jan 11, 2018

robingustafsson requested changes Jan 11, 2018

View reviewed changes

marklagendijk suggested changes Jan 17, 2018

View reviewed changes

cstyan force-pushed the callum-440 branch from aa897c7 to 4c11db9 Compare January 19, 2018 20:44

cstyan added 8 commits January 27, 2018 16:09

include the HTTP request info in responses

31e2e4d

use map[string][]string rather than directly using http.Header, the v…

57e13e0

…alue doesn't seem to be passed to JS code properly

add test for http request data within the http response struct

4fd5cbd

change HTTPRequest URL to string to match HTTPResponse struct

98dbaa5

rename HTTPReq var to respReq

846b86f

support multiple cookies with the same name in request struct

cf3d1a4

refactor setRequestCookies, add mergeCookies

d2e0837

refactor response structs handling of the requests body, add test

607866b

cstyan force-pushed the callum-440 branch from 4c11db9 to 607866b Compare January 28, 2018 00:24

robingustafsson approved these changes Jan 29, 2018

View reviewed changes

robingustafsson merged commit b8daff1 into grafana:master Jan 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose HTTP request information in response #447

Expose HTTP request information in response #447

cstyan commented Jan 11, 2018 •

edited

Loading

codecov-io commented Jan 11, 2018 •

edited

Loading

marklagendijk commented Jan 11, 2018

robingustafsson left a comment

robingustafsson Jan 11, 2018

cstyan Jan 12, 2018

robingustafsson Jan 11, 2018

robingustafsson Jan 11, 2018

robingustafsson Jan 11, 2018

cstyan Jan 12, 2018

robingustafsson Jan 12, 2018

robingustafsson Jan 11, 2018

cstyan Jan 12, 2018

cstyan commented Jan 16, 2018

marklagendijk commented Jan 16, 2018

cstyan commented Jan 17, 2018

marklagendijk Jan 17, 2018

marklagendijk Jan 17, 2018

cstyan Jan 17, 2018

cstyan commented Jan 28, 2018

robingustafsson commented Jan 29, 2018

		@@ -62,6 +62,8 @@ type HTTPResponse struct {
		OCSP OCSP `js:"ocsp"`
		Error string

Expose HTTP request information in response #447

Expose HTTP request information in response #447

Conversation

cstyan commented Jan 11, 2018 • edited Loading

codecov-io commented Jan 11, 2018 • edited Loading

Codecov Report

marklagendijk commented Jan 11, 2018

robingustafsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstyan commented Jan 16, 2018

marklagendijk commented Jan 16, 2018

cstyan commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstyan commented Jan 28, 2018

robingustafsson commented Jan 29, 2018

cstyan commented Jan 11, 2018 •

edited

Loading

codecov-io commented Jan 11, 2018 •

edited

Loading