Introduce SeatNonBid in hookoutcome

[ashishshinde-pubm]

Currently, bids getting rejected in module stage are not captured in seatNonBid. This PR introduce the SeatNonBid in HookStageOutcome so that each module stage can return rejected bids. In auction/amp_auction, we will collect the SeatNonBid from all stageOutcomes (stageOutcomes whose execution-status is success)

Changes in opertb_ext package -

1. To remove the cyclic dependency, moved the seat_non_bid.go and test files in openrtb_ext package.
2. Removed the dependency from 'entities.PbsOrtbBid' structure and introduced 'NonBidParams' struct inside openrtb_ext package.
3. Introduced Append() method in seat_non_bid.go which will append nonBids passed in input argument.
4. Each stageOutcome will contain seatNonBid object and we will collect and add in the bidResponseExt just before sending the response.

Changes in exchange.go -

1. Exchange will keep its own copy of SeatNonBid to collect bids rejected in HoldAuction.
2. Since the SeatNonBid is not concurrent-safe, each go-routine (getallbids function) will have to return the SeatNonBid and main thread will use Append() method to collect all rejected bids.
3. This SeatNonBid will be returned in the AuctionResponse.
4. Exchange will not add the SeatNonBids under the bidresponse.ext.prebid.seatnonbid because auctionresponsehook gets executed after holdAuction function in the auction/amp-auction, hence auction will take of adding all seatnonbids in bidresponseExt.

Changes in auction.go/amp_auction.go -

1. For each request, we will maintain SeatNonBid to collect rejected bids.
2. We will append the SeatNonBid returned by exchange (holdAuction).
3. In happy path, we will collect the SeatNonBid from all stageOutcomes and will append it to SeatNonBid got from exchange.
4. Based on flag request.ext.prebid.returnallbidstatus will decide and add the SeatNonBid in the bidResponseExt.
5. In case of critical error, ao.AuctionResponse/ao.Response would be nil hence we will collect SeatNonBid from stageOutcomes in the defer statement.

Changes in auction.go/video_auction.go -

1. Currently, Hook execution does not take place in the video_auction hence we will get SeatNonBid only from exchange (holdAuction).

Changes for unit-test cases -

1. Introduced mockSeatNonBidHook so that we can get the SeatNonBids from hookStageOutcomes for unit-test cases.
2. Add unit test cases wherever required either by extending existing function or by adding new one.

[ashishshinde-pubm]

@VeronikaSolovei9 We should call getNonBidsFromStageOutcomes only once. We cannot rely on ao.Errors. As per current implementation, we are not capturing the errors (ao.errors) but we need to call getNonBidsFromStageOutcomes and add ao.SeatNonBid for following cases .

1. https://github.com/ashishshinde-pubm/prebid-server/blob/SNBR_HookStage/endpoints/openrtb2/auction.go#L202
2. https://github.com/ashishshinde-pubm/prebid-server/blob/SNBR_HookStage/endpoints/openrtb2/auction.go#L287

Also, there are following cases where ao.Errors is not-empty and we don't want to call getNonBidsFromStageOutcomes in defer. (in this cases, we have already called the getNonBidsFromStageOutcomes )

1. https://github.com/ashishshinde-pubm/prebid-server/blob/SNBR_HookStage/endpoints/openrtb2/auction.go#L396
2. https://github.com/ashishshinde-pubm/prebid-server/blob/SNBR_HookStage/endpoints/openrtb2/auction.go#L417

As per current implementation, my requirement for auction.go is to call getNonBidsFromStageOutcomes in defer only when request doesn't pass through sendAuctionResponse function.

For amp_auction.go , we can rely on ao.Errors but to I was thinking to make both auction & amp_auction rely on common thing which seems to be ao.response in this case.

Thoughts ??

[VeronikaSolovei9]

@ashishshinde-pubm Thank you for your responses! We need to discuss something with the team and we will get back to you.

[ashishshinde-pubm]

@SyntaxNode , @VeronikaSolovei9 Any updates on this ?

[bsardo]

@ashishshinde-pubm sorry for the delay. There are a few other high priority items ahead of this but we will revisit soon.

[hhhjort]

There are also some merge conflicts now.

[zhongshixi]

@bsardo was looking at the PR, I also have a use case to construct SeatNonBid from our hook, i was more thinking of directly modifying the []SeatNonBid so hook that deals with openrtb.BidResponse can directly access ext.prebid.seatnobid to append bids that is rejected by hook ( since not all stages should construct []SeatNonBid, it should be handled after we receive bid from adapter ) , so i think we should provide an easy function/struct/interface to directly append []SeatNonBid in the original BidResponse which should save us a lot of trouble from modifying code in other places

e.g

we can then invoke the function in AddMutation function in the hook so we can append the seat non bid in concurrently safe way

let me know how the discussion goes so that you guys figure out a agreed workflow for the scenario

[ashishshinde-pubm]

@zhongshixi As per the PRD (https://github.com/prebid/prebid-server/issues/2367) , the seatnonbid are not just related to bid response. Example - SeatNonBid should be constructed for code-204 (Request Blocked - Privacy). Refer - openrtb_ext/seat_non_bids.go, it provides easy functions (NewBid/AddBid) for adding seatNonBids in bidResponse.

[zhongshixi]

@ashishshinde-pubm i see, thanks for the quick response, i read the PRD and i see the community's direction to include the bidder request as part of seat non bid. since this PR is only surfacing SeatNonBid in hookoutcome, i will post my thoughts on that PRD threads.

[zhongshixi]

@ashishshinde-pubm - I read through your PR and i like the design of using openrtb_ext.NonBidCollection{}, since i need to implement the feature of collecting rejected bidder request, i will borrow your idea of openrtb_ext.NonBidCollection{} only and use it in my PR

if you want, we can work on it together to make the puzzle complete so that seatNonBid can collect information on critical decision points in prebid server that rejects bid request and bid response

[ashishshinde-pubm]

@bsardo , @hhhjort , @VeronikaSolovei9 I've resolved the conflcts, Please have a look on this PR.

[ashishshinde-pubm]

@bretg , @bsardo Any plan to review this PR ?

[Pubmatic-Dhruv-Sonone]

@bsardo Addressed the review comments.