requestheaders: new parameter inside debug.httpcalls.<BIDDER> to log request header details #1659
Conversation
| RequestBody: string(httpInfo.request.Body), | ||
| Uri: httpInfo.request.Uri, | ||
| RequestBody: string(httpInfo.request.Body), | ||
| RequestHeaders: httpInfo.request.Headers, |
There was a problem hiding this comment.
Log Request Headers when there is error
| @@ -910,6 +916,7 @@ func TestBadResponseLogging(t *testing.T) { | |||
| if ext.Status != 0 { | |||
| t.Errorf("The Status code should be 0. Got %d", ext.Status) | |||
| } | |||
| assert.Equal(t, info.request.Headers, http.Header(ext.RequestHeaders), "The request headers should be \"header-1:value-1\"") | |||
There was a problem hiding this comment.
Verify Request headers are logged when there is a bad response
| Headers: http.Header{ | ||
| "header-1": []string{"value-1"}, | ||
| }, |
There was a problem hiding this comment.
Simulation of request headers set
| Headers: http.Header{ | ||
| "header-1": []string{"value-1", "value-2"}, | ||
| }, |
There was a problem hiding this comment.
Simulation of request headers set
| @@ -937,6 +947,7 @@ func TestSuccessfulResponseLogging(t *testing.T) { | |||
| if ext.Status != info.response.StatusCode { | |||
| t.Errorf("The Status code should be 0. Got %d", ext.Status) | |||
| } | |||
| assert.Equal(t, info.request.Headers, http.Header(ext.RequestHeaders), "The request headers should be \"%s\". Got %s", info.request.Headers, ext.RequestHeaders) | |||
There was a problem hiding this comment.
Verify Request headers are logged when there is a bad response
| @@ -65,6 +65,7 @@ | |||
| { | |||
| "uri": "appnexusTest.com", | |||
| "requestbody": "appnexusTestRequestBody", | |||
| "requestheaders": null, | |||
There was a problem hiding this comment.
Added new parameter to satisfy the unit test
| @@ -150,15 +152,17 @@ | |||
| "uri": "appnexusTest.com", | |||
| "requestbody": "appnexusTestRequestBody", | |||
| "responsebody": "appnexusTestResponseBody", | |||
| "status": 200 | |||
| "status": 200, | |||
| "requestheaders": null | |||
There was a problem hiding this comment.
Added new parameter to satisfy the unit test
| } | ||
| ], | ||
| "audienceNetwork": [ | ||
| { | ||
| "uri": "audienceNetworkTest.com", | ||
| "requestbody": "audienceNetworkTestRequestBody", | ||
| "responsebody": "audienceNetworkTestResponseBody", | ||
| "status": 200 | ||
| "status": 200, | ||
| "requestheaders": null |
There was a problem hiding this comment.
Added new parameter to satisfy the unit test
openrtb_ext/response.go
Outdated
| RequestBody string `json:"requestbody"` | ||
| ResponseBody string `json:"responsebody"` | ||
| Status int `json:"status"` | ||
| RequestHeaders map[string][]string `json:"requestheaders"` |
There was a problem hiding this comment.
The new parameter RequestHeaders will contain the header key and multiple values associated with it.
We have kept value as an array of strings because one header can have multiple values.
Also, this data structure is inline with http/Header
requestheaders is the key used while forming the JSON
|
@SyntaxNode : Please review and merge |
It's in our queue to review early next week. |
There was a problem hiding this comment.
Looks pretty good. Could we create an additional JSON test where "requestheaders" is not null? Say a file probably named exchange/exchangetest/request-debug-info-with-headers.json where we can assert that the request headers where correctly marshalled into the response.
…der information. Modified sample mock response such that ext.debug.httpcalls.appnexus.requestheaders will return the information of passed request headers
There was a problem hiding this comment.
Looks pretty good @ShriprasadM . I believe the only thing pending is @SyntaxNode 's comment on exchange/bidder_test.go line 889
Also Moved RequestHeaders next to RequestBidy in openrtb_ext.ExtHttpCall
|
@SyntaxNode : I have addressed the review comment I have also moved Do we need any documentation update around it ? @hhhjort : Thanks for review |
| Status int `json:"status"` | ||
| Uri string `json:"uri"` | ||
| RequestBody string `json:"requestbody"` | ||
| RequestHeaders map[string][]string `json:"requestheaders"` |
There was a problem hiding this comment.
Moved RequestHeaders next to RequestBody
|
@SyntaxNode : Can you review and approve? |
|
Thanks @SyntaxNode , @guscarreon and @hhhjort Do we need any documentation update around it ? |
httpcalls.<BIDDER>.requestheaders - New Debug parameter
@SyntaxNode, Please review and approve this PR.
It will be useful to log a set of request headers sent to the actual Bidder. This will help in verifying if required request headers are correctly getting sent to the actual Bidder.
This parameter will be visible only when query parameter debug=1 is set. For example,
In above example requestheaders indicating 2 headers are sent to appnexus bidder.