Adds timeout notifications for Facebook #1182
Conversation
adapters/audienceNetwork/facebook.go
Outdated
|
|
||
| timeoutReq := adapters.RequestData{ | ||
| Method: "GET", | ||
| Uri: fmt.Sprintf("https://www.facebook.com/audiencenetwork/nurl/?partner=%s&app=%s&auction=%s&ortb_loss_code=2", fa.platformID, fa.platformID, auction_id), |
There was a problem hiding this comment.
Consider moving the uri construction up a level. IMHO the extra indentation doesn't make the string substitutions any easier to see.
adapters/bidder.go
Outdated
| @@ -39,6 +39,20 @@ type Bidder interface { | |||
| MakeBids(internalRequest *openrtb.BidRequest, externalRequest *RequestData, response *ResponseData) (*BidderResponse, []error) | |||
| } | |||
|
|
|||
| // TimeoutBidder is used to identify bidders that support timeout notifications. | |||
| type TimeoutBidder interface { | |||
| MakeRequests(request *openrtb.BidRequest, reqInfo *ExtraRequestInfo) ([]*RequestData, []error) | |||
There was a problem hiding this comment.
Why are the standard bidder methods duplicated here?
There was a problem hiding this comment.
Not 100% sure it is needed, but it does make clear that it is a bidder rather than some other random thing, and can still be used for bidder things.
|
We can only generate one timeout notification per timeout, and we already have metrics on timeout, so I don't see the need for an additional metric for explicit notifications |
exchange/bidder.go
Outdated
| httpReq.Header = req.Headers | ||
| httpResp, _ := ctxhttp.Do(ctx, bidder.Client, httpReq) | ||
| // No validation yet on sending notifications | ||
| _ = httpResp |
There was a problem hiding this comment.
Why are we doing this assighnment? Is the compiler complaining about the variable being declared and not used? If that's the case, should we create it in the first place? What if we just leave it blank?
341 httpReq.Header = req.Headers
342 - httpResp, _ := ctxhttp.Do(ctx, bidder.Client, httpReq)
+ _, _ := ctxhttp.Do(ctx, bidder.Client, httpReq)
343 - // No validation yet on sending notifications
344 - _ = httpResp
There was a problem hiding this comment.
Yes, that is probably best. I had pondered doing something with the response, but then decided against it.
|
|
||
| func TestMakeTimeoutNotice(t *testing.T) { | ||
| req := adapters.RequestData{ | ||
| Body: []byte(`{"imp":[{"id":"1234"}]}}`), |
There was a problem hiding this comment.
Could we also add another test case for an invalid json in this test?
No description provided.