Skip to content
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
merged 43 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 39 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 Jul 3, 2023
60504ef
i2852: Corrected the errorcode and resolved unit test issues
pm-shriprasad-marathe Jul 3, 2023
a48cd10
Added unit tests and argument refactoring
pm-shriprasad-marathe Jul 12, 2023
d4754a1
Fixed unit tests
pm-shriprasad-marathe Jul 12, 2023
85825ba
Reverted unwanted change
pm-shriprasad-marathe Jul 13, 2023
449d59c
added few more assertions
pm-shriprasad-marathe Jul 13, 2023
5739ad0
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe Jul 26, 2023
f7a498f
i2852: resolved the conflicts
pm-shriprasad-marathe Jul 26, 2023
3e9b4f6
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe May 7, 2024
eaef765
i2852: Refactored code to captured Non bid reasons when bidder has er…
pm-shriprasad-marathe Jun 3, 2024
3210501
i2852: Assigned adapter non bids to global object
pm-shriprasad-marathe Jun 3, 2024
113547b
i2852: Added new transformers to convert httpInfo.err into non bid re…
pm-shriprasad-marathe Jun 3, 2024
4fd699e
i2852: Added new method buildProxyNonBids to build multiple proxy non…
pm-shriprasad-marathe Jun 3, 2024
6b39d39
i2852: Added omit empty
pm-shriprasad-marathe Jun 3, 2024
ef0b92c
i2852: Added and modified unit tests
pm-shriprasad-marathe Jun 7, 2024
b5dc482
i2852: Fixed Unit Test Issue
pm-shriprasad-marathe Jun 7, 2024
3d83e65
Addressed code review comments
pm-shriprasad-marathe Jun 18, 2024
dba6327
Addressed review comments
pm-shriprasad-marathe Jul 2, 2024
03902eb
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe Jul 2, 2024
9e3d26f
Removed code which was not part of this PR
pm-shriprasad-marathe Jul 3, 2024
14216fa
Changed extension objects to pointers
pm-shriprasad-marathe Jul 3, 2024
9065ebc
set seatnonbids objects only if nonbids are exists
pm-shriprasad-marathe Jul 3, 2024
0b683d4
Fixed UTs. Revered *json.RawMessage to json.RawMessage
pm-shriprasad-marathe Jul 4, 2024
71107d3
Fixed the multiple seatnonbid objects per seat
pm-shriprasad-marathe Jul 11, 2024
a136fe5
UT fixes
pm-shriprasad-marathe Jul 11, 2024
ffb78aa
Added handling for no route to host and comment
pm-shriprasad-marathe Jul 11, 2024
5d0ad0f
Added comment and reverted the unwanted config from test.json
pm-shriprasad-marathe Jul 15, 2024
2c1eee9
Merge pull request #846 from PubMatic-OpenWrap/i2852_timeout_nonbidre…
ShriprasadM Jul 15, 2024
0260f1c
Removed unwanted comment and using `rejectImps` function name
pm-shriprasad-marathe Aug 5, 2024
8dea49f
Using context.Background() now
pm-shriprasad-marathe Aug 5, 2024
9de20c4
Corrected ordering and refactoring as per review comments
pm-shriprasad-marathe Aug 5, 2024
4f3838f
Renamed tests and added new test for no route to host
pm-shriprasad-marathe Aug 5, 2024
c67d074
Renamed addProxyNonBids to rejectImps
pm-shriprasad-marathe Aug 5, 2024
e76c81e
Removed commented code
pm-shriprasad-marathe Aug 5, 2024
ecb7c12
Merge remote-tracking branch 'upstream/master' into i2852_timeout_non…
pm-shriprasad-marathe Aug 20, 2024
4a2dd48
I2852 enumerations refactor (#886)
ShriprasadM Aug 31, 2024
6845084
Merge branch 'master' of github.com:prebid/prebid-server into i2852_t…
ashishshinde-pubm Aug 31, 2024
a966b1a
Merge branch 'i2852_timeout_nonbidreason' of github.com:PubMatic-Open…
ashishshinde-pubm Aug 31, 2024
720c768
address review comments
ashishshinde-pubm Aug 31, 2024
e4bc745
Adding Unit tests
Pubmatic-Dhruv-Sonone Sep 11, 2024
4c31793
Updating unit test
Pubmatic-Dhruv-Sonone Sep 11, 2024
656f109
Updating Unit test
Pubmatic-Dhruv-Sonone Sep 12, 2024
ef4c3d4
Merge branch 'master' of github.com:prebid/prebid-server into i2852_t…
ashishshinde-pubm Sep 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion analytics/pubstack/pubstack_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestNewModuleSuccess(t *testing.T) {
{
ImpId: "123",
StatusCode: 34,
Ext: openrtb_ext.NonBidExt{Prebid: openrtb_ext.ExtResponseNonBidPrebid{Bid: openrtb_ext.NonBidObject{}}},
Ext: &openrtb_ext.NonBidExt{Prebid: openrtb_ext.ExtResponseNonBidPrebid{Bid: openrtb_ext.NonBidObject{}}},
},
},
},
Expand Down
8 changes: 8 additions & 0 deletions exchange/bidder.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,14 @@ type bidRequestOptions struct {

type extraBidderRespInfo struct {
respProcessingStartTime time.Time
seatNonBidBuilder SeatNonBidBuilder
}

type extraAuctionResponseInfo struct {
fledge *openrtb_ext.Fledge
bidsFound bool
bidderResponseStartTime time.Time
seatNonBidBuilder SeatNonBidBuilder
}

const ImpIdReqBody = "Stored bid response for impression id: "
Expand Down Expand Up @@ -135,6 +137,7 @@ type bidderAdapterConfig struct {
func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest BidderRequest, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo, adsCertSigner adscert.Signer, bidRequestOptions bidRequestOptions, alternateBidderCodes openrtb_ext.ExtAlternateBidderCodes, hookExecutor hookexecution.StageExecutor, ruleToAdjustments openrtb_ext.AdjustmentsByDealID) ([]*entities.PbsOrtbSeatBid, extraBidderRespInfo, []error) {
request := openrtb_ext.RequestWrapper{BidRequest: bidderRequest.BidRequest}
reject := hookExecutor.ExecuteBidderRequestStage(&request, string(bidderRequest.BidderName))
var seatNonBidBuilder SeatNonBidBuilder
if reject != nil {
return nil, extraBidderRespInfo{}, []error{reject}
}
Expand Down Expand Up @@ -198,6 +201,7 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest Bidde
// If the bidder only needs to make one, save some cycles by just using the current one.
dataLen = len(reqData) + len(bidderRequest.BidderStoredResponses)
responseChannel = make(chan *httpCallInfo, dataLen)
seatNonBidBuilder = SeatNonBidBuilder{}
if len(reqData) == 1 {
responseChannel <- bidder.doRequest(ctx, reqData[0], bidRequestOptions.bidderRequestStartTime, bidRequestOptions.tmaxAdjustments)
} else {
Expand Down Expand Up @@ -398,13 +402,17 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest Bidde
}
} else {
errs = append(errs, httpInfo.err)
nonBidReason := httpInfoToNonBidReason(httpInfo)
seatNonBidBuilder.rejectImps(httpInfo.request.ImpIDs, nonBidReason, string(bidderRequest.BidderName))
}
}

seatBids := make([]*entities.PbsOrtbSeatBid, 0, len(seatBidMap))
for _, seatBid := range seatBidMap {
seatBids = append(seatBids, seatBid)
}

extraRespInfo.seatNonBidBuilder = seatNonBidBuilder
Copy link
Collaborator

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?

Copy link
Contributor

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.

return seatBids, extraRespInfo, errs
}

Expand Down
146 changes: 146 additions & 0 deletions exchange/bidder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -3096,6 +3100,148 @@ func TestGetBidType(t *testing.T) {
}
}

func TestSeatNonBid(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
Loading
Loading