-
Notifications
You must be signed in to change notification settings - Fork 769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log non bid reasons in bidder framework #2891
Merged
bsardo
merged 43 commits into
prebid:master
from
PubMatic-OpenWrap:i2852_timeout_nonbidreason
Sep 19, 2024
Merged
Changes from 40 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
96c4e18
i2852: Added support in bidder framework to log non bid reasons
pm-shriprasad-marathe 60504ef
i2852: Corrected the errorcode and resolved unit test issues
pm-shriprasad-marathe a48cd10
Added unit tests and argument refactoring
pm-shriprasad-marathe d4754a1
Fixed unit tests
pm-shriprasad-marathe 85825ba
Reverted unwanted change
pm-shriprasad-marathe 449d59c
added few more assertions
pm-shriprasad-marathe 5739ad0
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe f7a498f
i2852: resolved the conflicts
pm-shriprasad-marathe 3e9b4f6
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe eaef765
i2852: Refactored code to captured Non bid reasons when bidder has er…
pm-shriprasad-marathe 3210501
i2852: Assigned adapter non bids to global object
pm-shriprasad-marathe 113547b
i2852: Added new transformers to convert httpInfo.err into non bid re…
pm-shriprasad-marathe 4fd699e
i2852: Added new method buildProxyNonBids to build multiple proxy non…
pm-shriprasad-marathe 6b39d39
i2852: Added omit empty
pm-shriprasad-marathe ef0b92c
i2852: Added and modified unit tests
pm-shriprasad-marathe b5dc482
i2852: Fixed Unit Test Issue
pm-shriprasad-marathe 3d83e65
Addressed code review comments
pm-shriprasad-marathe dba6327
Addressed review comments
pm-shriprasad-marathe 03902eb
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe 9e3d26f
Removed code which was not part of this PR
pm-shriprasad-marathe 14216fa
Changed extension objects to pointers
pm-shriprasad-marathe 9065ebc
set seatnonbids objects only if nonbids are exists
pm-shriprasad-marathe 0b683d4
Fixed UTs. Revered *json.RawMessage to json.RawMessage
pm-shriprasad-marathe 71107d3
Fixed the multiple seatnonbid objects per seat
pm-shriprasad-marathe a136fe5
UT fixes
pm-shriprasad-marathe ffb78aa
Added handling for no route to host and comment
pm-shriprasad-marathe 5d0ad0f
Added comment and reverted the unwanted config from test.json
pm-shriprasad-marathe 2c1eee9
Merge pull request #846 from PubMatic-OpenWrap/i2852_timeout_nonbidre…
ShriprasadM 0260f1c
Removed unwanted comment and using `rejectImps` function name
pm-shriprasad-marathe 8dea49f
Using context.Background() now
pm-shriprasad-marathe 9de20c4
Corrected ordering and refactoring as per review comments
pm-shriprasad-marathe 4f3838f
Renamed tests and added new test for no route to host
pm-shriprasad-marathe c67d074
Renamed addProxyNonBids to rejectImps
pm-shriprasad-marathe e76c81e
Removed commented code
pm-shriprasad-marathe ecb7c12
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe 4a2dd48
I2852 enumerations refactor (#886)
ShriprasadM 6845084
Merge branch 'master' of github.com:prebid/prebid-server into i2852_t…
ashishshinde-pubm a966b1a
Merge branch 'i2852_timeout_nonbidreason' of github.com:PubMatic-Open…
ashishshinde-pubm 720c768
address review comments
ashishshinde-pubm e4bc745
Adding Unit tests
Pubmatic-Dhruv-Sonone 4c31793
Updating unit test
Pubmatic-Dhruv-Sonone 656f109
Updating Unit test
Pubmatic-Dhruv-Sonone ef4c3d4
Merge branch 'master' of github.com:prebid/prebid-server into i2852_t…
ashishshinde-pubm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,11 +9,15 @@ import ( | |
"errors" | ||
"fmt" | ||
"io" | ||
"net" | ||
"net/http" | ||
"net/http/httptest" | ||
"net/http/httptrace" | ||
"net/url" | ||
"os" | ||
"sort" | ||
"strings" | ||
"syscall" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -3096,6 +3100,148 @@ func TestGetBidType(t *testing.T) { | |
} | ||
} | ||
|
||
func TestSeatNonBid(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any other test cases you can think of that would make sense here? I only ask since I see only one test case and was wondering if there are any others that would make sense. |
||
type args struct { | ||
BidRequest *openrtb2.BidRequest | ||
Seat string | ||
SeatRequests []*adapters.RequestData | ||
BidderResponse func() (*http.Response, error) | ||
client *http.Client | ||
} | ||
type expect struct { | ||
seatBids []*entities.PbsOrtbSeatBid | ||
seatNonBids SeatNonBidBuilder | ||
errors []error | ||
} | ||
testCases := []struct { | ||
name string | ||
args args | ||
expect expect | ||
}{ | ||
{ | ||
name: "NBR_101_timeout_for_context_deadline_exceeded", | ||
args: args{ | ||
Seat: "pubmatic", | ||
BidRequest: &openrtb2.BidRequest{ | ||
Imp: []openrtb2.Imp{{ID: "1234"}}, | ||
}, | ||
SeatRequests: []*adapters.RequestData{{ImpIDs: []string{"1234"}}}, | ||
BidderResponse: func() (*http.Response, error) { return nil, context.DeadlineExceeded }, | ||
client: &http.Client{Timeout: time.Nanosecond}, // for timeout | ||
}, | ||
expect: expect{ | ||
seatNonBids: SeatNonBidBuilder{ | ||
"pubmatic": {{ | ||
ImpId: "1234", | ||
StatusCode: int(ErrorTimeout), | ||
}}, | ||
}, | ||
errors: []error{&errortypes.Timeout{Message: context.DeadlineExceeded.Error()}}, | ||
seatBids: []*entities.PbsOrtbSeatBid{{Bids: []*entities.PbsOrtbBid{}, Currency: "USD", Seat: "pubmatic", HttpCalls: []*openrtb_ext.ExtHttpCall{}}}, | ||
}, | ||
}, { | ||
name: "NBR_103_Bidder_Unreachable_Connection_Refused", | ||
args: args{ | ||
Seat: "appnexus", | ||
SeatRequests: []*adapters.RequestData{{ImpIDs: []string{"1234", "4567"}}}, | ||
BidRequest: &openrtb2.BidRequest{Imp: []openrtb2.Imp{{ID: "1234"}, {ID: "4567"}}}, | ||
BidderResponse: func() (*http.Response, error) { | ||
return nil, &net.OpError{Err: os.NewSyscallError(syscall.ECONNREFUSED.Error(), syscall.ECONNREFUSED)} | ||
}, | ||
}, | ||
expect: expect{ | ||
seatNonBids: SeatNonBidBuilder{ | ||
"appnexus": { | ||
{ImpId: "1234", StatusCode: int(ErrorBidderUnreachable)}, | ||
{ImpId: "4567", StatusCode: int(ErrorBidderUnreachable)}, | ||
}, | ||
}, | ||
seatBids: []*entities.PbsOrtbSeatBid{{Bids: []*entities.PbsOrtbBid{}, Currency: "USD", Seat: "appnexus", HttpCalls: []*openrtb_ext.ExtHttpCall{}}}, | ||
errors: []error{&url.Error{Op: "Get", URL: "", Err: &net.OpError{Err: os.NewSyscallError(syscall.ECONNREFUSED.Error(), syscall.ECONNREFUSED)}}}, | ||
}, | ||
}, { | ||
name: "no_impids_populated_in_request_data", | ||
args: args{ | ||
SeatRequests: []*adapters.RequestData{{ | ||
ImpIDs: nil, // no imp ids | ||
}}, | ||
BidRequest: &openrtb2.BidRequest{Imp: []openrtb2.Imp{{ID: "1234"}}}, | ||
BidderResponse: func() (*http.Response, error) { | ||
return nil, errors.New("some_error") | ||
}, | ||
}, | ||
expect: expect{ | ||
seatNonBids: SeatNonBidBuilder{}, | ||
seatBids: []*entities.PbsOrtbSeatBid{{Bids: []*entities.PbsOrtbBid{}, Currency: "USD", HttpCalls: []*openrtb_ext.ExtHttpCall{}}}, | ||
errors: []error{&url.Error{Op: "Get", URL: "", Err: errors.New("some_error")}}, | ||
}, | ||
}, | ||
} | ||
for _, test := range testCases { | ||
t.Run(test.name, func(t *testing.T) { | ||
mockBidder := &mockBidder{} | ||
mockBidder.On("MakeRequests", mock.Anything, mock.Anything).Return(test.args.SeatRequests, []error(nil)) | ||
mockMetricsEngine := &metrics.MetricsEngineMock{} | ||
mockMetricsEngine.On("RecordOverheadTime", mock.Anything, mock.Anything).Return(nil) | ||
mockMetricsEngine.On("RecordBidderServerResponseTime", mock.Anything).Return(nil) | ||
roundTrip := &mockRoundTripper{} | ||
roundTrip.On("RoundTrip", mock.Anything).Return(test.args.BidderResponse()) | ||
client := &http.Client{ | ||
Transport: roundTrip, | ||
Timeout: 0, | ||
} | ||
if test.args.client != nil { | ||
client.Timeout = test.args.client.Timeout | ||
} | ||
bidder := AdaptBidder(mockBidder, client, &config.Configuration{}, mockMetricsEngine, openrtb_ext.BidderAppnexus, &config.DebugInfo{}, test.args.Seat) | ||
|
||
ctx := context.Background() | ||
if client.Timeout > 0 { | ||
ctxTimeout, cancel := context.WithTimeout(ctx, client.Timeout) | ||
ctx = ctxTimeout | ||
defer cancel() | ||
} | ||
seatBids, responseExtra, errors := bidder.requestBid(ctx, BidderRequest{ | ||
BidRequest: test.args.BidRequest, | ||
BidderName: openrtb_ext.BidderName(test.args.Seat), | ||
}, nil, &adapters.ExtraRequestInfo{}, &MockSigner{}, bidRequestOptions{}, openrtb_ext.ExtAlternateBidderCodes{}, hookexecution.EmptyHookExecutor{}, nil) | ||
assert.Equal(t, test.expect.seatBids, seatBids) | ||
assert.Equal(t, test.expect.seatNonBids, responseExtra.seatNonBidBuilder) | ||
assert.Equal(t, test.expect.errors, errors) | ||
for _, nonBids := range responseExtra.seatNonBidBuilder { | ||
for _, nonBid := range nonBids { | ||
for _, seatBid := range seatBids { | ||
for _, bid := range seatBid.Bids { | ||
// ensure non bids are not present in seat bids | ||
if nonBid.ImpId == bid.Bid.ImpID { | ||
assert.Fail(t, "imp id [%s] present in both seat bid and non seat bid", nonBid.ImpId) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
}) | ||
} | ||
} | ||
|
||
type mockRoundTripper struct { | ||
mock.Mock | ||
} | ||
|
||
func (rt *mockRoundTripper) RoundTrip(request *http.Request) (*http.Response, error) { | ||
args := rt.Called(request) | ||
var response *http.Response | ||
if args.Get(0) != nil { | ||
response = args.Get(0).(*http.Response) | ||
} | ||
var err error | ||
if args.Get(1) != nil { | ||
err = args.Get(1).(error) | ||
} | ||
|
||
return response, err | ||
} | ||
|
||
type mockBidderTmaxCtx struct { | ||
startTime, deadline, now time.Time | ||
ok bool | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this will be nil if there aren't any imps. If there is at least one imp, this will be an empty map or a map with entries if there was at least one http error. Is that correct?
I'm thinking that it might be easier if
seatNonBidBuilder
is always not nil. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialised the seatNonBidBuilder to empty map.