Introduce SeatNonBid in hookoutcome #3416

ashishshinde-pubm · 2024-01-19T03:44:31Z

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 -

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

Changes in exchange.go -

Exchange will keep its own copy of SeatNonBid to collect bids rejected in HoldAuction.
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.
This SeatNonBid will be returned in the AuctionResponse.
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 -

For each request, we will maintain SeatNonBid to collect rejected bids.
We will append the SeatNonBid returned by exchange (holdAuction).
In happy path, we will collect the SeatNonBid from all stageOutcomes and will append it to SeatNonBid got from exchange.
Based on flag request.ext.prebid.returnallbidstatus will decide and add the SeatNonBid in the bidResponseExt.
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 -

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 -

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

…nto SNBR_HookStage

SyntaxNode · 2024-01-19T17:15:52Z

openrtb_ext/seat_non_bids.go

+// NonBidsWrapper contains the map of seat with list of nonBids
+type NonBidsWrapper struct {
+	seatNonBidsMap map[string][]NonBid
+}


We use the term 'wrapper" in this package for helper objects which assist with json encoding/decoding. This is a different usage. Please use a different term, such as something like NonBidCollection.

ok, changed it to NonBidCollection

SyntaxNode · 2024-01-19T17:18:49Z

openrtb_ext/seat_non_bids.go

+
+// AddBid adds the nonBid into the map against the respective seat.
+// Note: This function is not a thread safe.
+func (snb *NonBidsWrapper) AddBid(bidParams NonBidParams) {


This method has two responsibilities, to build a NonBid object and add it to the collection. I recommend to separate these responsibilities. A helper function named something like NewNonBid or BuildNonBid could be encapsulate building a NonBid from a combination of the bid, reason, seat, etc...

done, introduced new function NewNonBid

SyntaxNode · 2024-01-19T17:55:35Z

openrtb_ext/seat_non_bids.go

+		ImpId:      bidParams.Bid.ImpID,
+		StatusCode: bidParams.NonBidReason,
+		Ext: NonBidExt{
+			Prebid: ExtResponseNonBidPrebid{Bid: NonBidObject{


It's strange to see a type called NonBidObject instead of simply NonBid. I'd like the names to be clearer.

We often prepend Ext for structures contained within an extension field. However, Seat Non Bid is intended to be promoted to the top level response in the future so I support the existing names of SeatNonBid and NonBid. These names would remain the same when the structures are moved into the OpenRTB repo.

I suggest we rename NonBidExt to ExtNonBid to better align with the naming pattern used for other objects. ExtReponseNonBidPrebid can be shorted to ExtNonBidPrebid for improved readability and to enforce the relationship with it's parent ExtNonBid structure. Then the unclear name NonBidObject can be renamed to ExtNonBidPrebidBid which is a little wordy but very clear.

Renamed the variables like below
NonBidExt ------> ExtNonBid
ExtReponseNonBidPrebid ----> ExtNonBidPrebid
NonBidObject ---> ExtNonBidPrebidBid

SyntaxNode · 2024-01-19T17:59:55Z

endpoints/openrtb2/amp_auction.go

@@ -138,6 +139,12 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 	activityControl := privacy.ActivityControl{}

 	defer func() {
+		// if AmpObject.AuctionResponse is nil then collect nonbids from all stage outcomes and set it in the AmpObject.SeatNonBid
+		// Nil AmpObject.AuctionResponse indicates the occurrence of a fatal error.
+		if ao.AuctionResponse == nil {


Is there a more direct way for checking the occurrence of a fatal error instead of needing to explain the significance of an empty response in a comment? Hopefully one we can reuse in other parts of the code instead of copying/pasting this comment.

One direct way would be to call below 2 lines after every return statement.
seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
ao.SeatNonBid = seatNonBid.Get()

But currently there are around 6/7 return statement in Auction() & AmpAuction() function. To avoid the repetitive code, added this block in defer statement.
Thoughts ??

I think defer block is a good place for this.
Can we rely on the errors in ao? Looks like fatal errors should be added in ao in case of every error in the endpoint, like: ao.Errors = append(ao.Errors, rejectErr)

Same for equivalent logic in "auction.go"

You could perhaps have a foundErrors boolean flag that starts false, and then is set to true whenever errors are detected. Then use this flag in the defer rather than checking for nil, resulting in a more straightforward way to satisfy the conditional, and futureproofing against any possible change where either an error happens after ao.AuctionResponse is set or it stays nil despite no error.

Used foundError instead of ao.Response nil check.

SyntaxNode · 2024-01-19T18:00:34Z

endpoints/openrtb2/amp_auction.go

@@ -325,6 +340,7 @@ func sendAmpResponse(
 	labels metrics.Labels,
 	ao analytics.AmpObject,
 	errs []error,
+	seatNonBid *openrtb_ext.NonBidsWrapper,


Why is this passed as a pointer / by reference?

No need to pass it as pointer. corrected it.

SyntaxNode · 2024-01-19T18:02:04Z

endpoints/openrtb2/amp_auction.go

@@ -352,6 +368,8 @@ func sendAmpResponse(
 						glog.Errorf("/openrtb2/amp Critical error unpacking targets: %v", err)
 						ao.Errors = append(ao.Errors, fmt.Errorf("Critical error while unpacking AMP targets: %v", err))
 						ao.Status = http.StatusInternalServerError
+						seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))


Related to my above comment, seatNonBid could be nil here.. leading to a confusing situation.

Agree, made correction. Now passing the seatNonBid as a copy.

SyntaxNode · 2024-01-19T18:07:09Z

hooks/hookexecution/outcome.go

+	DebugMessages []string                   `json:"debug_messages,omitempty"`
+	Errors        []string                   `json:"-"`
+	Warnings      []string                   `json:"-"`
+	SeatNonBid    openrtb_ext.NonBidsWrapper `json:"-"`


Why is this not a slice of SeatNonBid objects?

We believe, building the standard openrtb_ext.SeatNonBid is expensive because each nonbid needs to be mapped against respective seat and impid.
Similarly, obtaining standard openrtb_ext.SeatNonBid object from each stageOutcome and again building the final openrtb_ext.SeatNonBid would add additional computational overhead.
Example -
Assume, RawBidderResponse hook returns

SeatNonBid: []openrtb_ext.SeatNonBid{ { NonBid: []openrtb_ext.NonBid{ { ImpId: "imp1", StatusCode: 100, } }, Seat: "pubmatic", }

& AuctionResponseHook hook returns

SeatNonBid: []openrtb_ext.SeatNonBid{ { NonBid: []openrtb_ext.NonBid{ { ImpId: "imp2", StatusCode: 100, } }, Seat: "pubmatic", }

But we need to combine them for bidresponseExt.Prebid.SeatNonBid and auctionObject.SeatNonBid like

SeatNonBid: []openrtb_ext.SeatNonBid{ { NonBid: []openrtb_ext.NonBid{ { ImpId: "imp1", StatusCode: 100, }, { ImpId: "imp2", StatusCode: 100, } }, Seat: "pubmatic", }

But, if we allow hookOutcome to return openrtb_ext.NonBidsWrapper/openrtb_ext.NonBidCollection then it become easy to add non-bids by using SeatNonBid.AddBid() function.
To combine the SeatNonBids from multiple hookOutcomes, we can use SeatNonBid.Append() function.
Please refer - getNonBidsFromStageOutcomes function which does the same.

VeronikaSolovei9

Added mostly questions for better understanding the context.

VeronikaSolovei9 · 2024-01-27T23:44:22Z

endpoints/openrtb2/amp_auction.go

+		if ao.AuctionResponse != nil {
+			// this check ensures that we collect nonBids from stageOutcomes only once.
+			// there could be a case where ao.AuctionResponse nil and reqWrapper.RebuildRequest returns error
+			seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
+			ao.SeatNonBid = seatNonBid.Get()
+		}


Is this needed here? It duplicates logic from defer block

in defer, we are collecting non-bids only when ao.AuctionResponse is nil.
This is one place, where we can get the ao.AuctionResponse as non-nil and defer will not collect the non-bids.

VeronikaSolovei9 · 2024-01-27T23:44:47Z

endpoints/openrtb2/amp_auction.go

@@ -138,6 +139,12 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 	activityControl := privacy.ActivityControl{}

 	defer func() {
+		// if AmpObject.AuctionResponse is nil then collect nonbids from all stage outcomes and set it in the AmpObject.SeatNonBid
+		// Nil AmpObject.AuctionResponse indicates the occurrence of a fatal error.
+		if ao.AuctionResponse == nil {


I think defer block is a good place for this.
Can we rely on the errors in ao? Looks like fatal errors should be added in ao in case of every error in the endpoint, like: ao.Errors = append(ao.Errors, rejectErr)

Same for equivalent logic in "auction.go"

VeronikaSolovei9 · 2024-01-28T00:00:59Z

endpoints/openrtb2/auction_test.go

+		want        want
+	}{
+		{
+			description: "request parsing failed, auctionObject should contain seatNonBid",


This part is not clear to me. parseRequest has nothing to do with bid responses. Do we still want to include seatNonBid to the auction response?

As part of the SeatNonBid proposal (#2367) , seatNonBid is not just related to bid-responses.
Please refer NBR codes (202: Error - bad input parameters, 116: No Bid - Incomplete SupplyChain) this needs to be capture inside parseRequest.

VeronikaSolovei9 · 2024-01-28T00:22:06Z

endpoints/openrtb2/auction.go

+	stageOutcomes := hookExecutor.GetOutcomes()
+	seatNonBid.Append(getNonBidsFromStageOutcomes(stageOutcomes))
+	ao.SeatNonBid = seatNonBid.Get()


This code is also duplicating logic in defer. Func sendAuctionResponse executes right before the defer block. Do we need it here?

In sendAuctionResponse , we will always get the non-nil ao.Response.
In defer block, we are collecting nonBids from stageOutcomes only when ao.Response is nil.
We need it here, because we need to add the seatNonBids in resp.ext.prebid.seatnonbid

SyntaxNode · 2024-01-30T14:33:52Z

openrtb_ext/seat_non_bids.go

@@ -0,0 +1,80 @@
+package openrtb_ext
+
+import "github.com/prebid/openrtb/v19/openrtb2"


We updated the OpenRTB library to version 20. Please merge with the master branch and change these imports to "github.com/prebid/openrtb/v20/openrtb2"

ashishshinde-pubm · 2024-01-31T04:58:56Z

@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 .

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 )

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 · 2024-02-06T04:25:59Z

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

ashishshinde-pubm · 2024-02-19T05:18:22Z

@SyntaxNode , @VeronikaSolovei9 Any updates on this ?

bsardo · 2024-02-22T20:09:25Z

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

hhhjort · 2024-03-12T17:17:11Z

There are also some merge conflicts now.

zhongshixi · 2024-04-15T22:32:47Z

@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 · 2024-04-18T15:06:28Z

@zhongshixi As per the PRD (#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 · 2024-04-19T14:35:57Z

@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 · 2024-04-22T14:24:47Z

@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 · 2024-04-22T17:15:29Z

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

ashishshinde-pubm · 2024-05-15T02:50:02Z

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

…nto SNBR_HookStage

bsardo · 2024-09-09T01:56:25Z

openrtb_ext/seat_non_bids.go

+type NonBidCollection struct {
+	seatNonBidsMap map[string][]NonBid
+}


Can we get rid of NonBidCollection and this entire file and use SeatNonBidBuilder from #2891?

Remove NonBidCollection. But move SeatNonBidBuilder to openrtb_ext package from exchange package due to cyclic import.

bsardo · 2024-09-09T12:28:29Z

endpoints/openrtb2/auction.go

+		// if AuctionObject.Response is nil then collect nonbids from all stage outcomes and set it in the AuctionObject.
+		// Nil AuctionObject.Response indicates the occurrence of a fatal error.
+		if ao.Response == nil {
+			seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
+			ao.SeatNonBid = seatNonBid.Get()
+		}


While I understand why you chose to put this logic in defer, I don't think we should get into the habit of putting such conditional and computational logic in here. Is there another place we can put this?

To avoid adding this code at multiple places, added it in the defer block.

bsardo · 2024-09-09T12:35:25Z

endpoints/openrtb2/auction.go

+	if response.Ext == nil {
+		response.Ext = json.RawMessage(`{}`)
+	}


Why do we need to do this? Is this to avoid an error when unmarshaling?

bsardo · 2024-09-09T14:28:19Z

endpoints/openrtb2/auction.go

 		ao.HookExecutionOutcome = stageOutcomes
+		err := setSeatNonBidRaw(ao.RequestWrapper, response, ao.SeatNonBid)


Perhaps this is necessary at the moment but it seems a little odd that we are using the request wrapper that is on the analytics object.

RequestWrapper is not being passed to sendAuctionResponse currently. We will have to change sendAuctionResponse definition to accept RequestWrapper as argument instead of BidRequest.

Pubmatic-Dhruv-Sonone · 2024-10-21T05:57:46Z

@bsardo Addressed the review comments.

bsardo · 2024-11-04T17:58:34Z

Hi @Pubmatic-Dhruv-Sonone, can you please merge with master again to resolve conflicts? Thanks!

Pubmatic-Dhruv-Sonone · 2024-11-06T16:08:27Z

@bsardo Merged master. Please review again.

SyntaxNode · 2024-11-15T21:41:35Z

endpoints/openrtb2/auction.go

-	if setSeatNonBid(respExt, request, auctionResponse) {
-		if respExtJson, err := jsonutil.Marshal(respExt); err == nil {
+	if setSeatNonBid(respExt, nonBids) {
+		if respExtJson, err := json.Marshal(respExt); err == nil {


Please leave the usage of our jsonutil helper.

hhhjort · 2024-12-12T16:08:31Z

endpoints/openrtb2/auction_test.go

+			},
+		},
+		{
+			name: "returnallbistatus is true, update seatnonbid in nil responseExt",


typo in the name, should be "returnallbidstatus is true..."

Pubmatic-Dhruv-Sonone · 2024-12-25T12:12:10Z

@hhhjort Addressed the review comments. Please check.

ashishshinde-pubm added 4 commits January 6, 2024 13:18

Introduce SeatNonBid in hookoutcome

8678237

Merge branch 'master' of github.com:ashishshinde-pubm/prebid-server i…

acef141

…nto SNBR_HookStage

Move the call to nonbids.Append in defer

c6525bc

Merge branch 'master' of github.com/prebid-server into SNBR_HookStage

47b5c6f

ashishshinde-pubm marked this pull request as ready for review January 19, 2024 03:49

SyntaxNode reviewed Jan 19, 2024

View reviewed changes

ashishshinde-pubm added 2 commits January 21, 2024 19:22

Address review comments

ffe2748

introduce NewNonBid method

72e625e

ashishshinde-pubm requested a review from SyntaxNode January 23, 2024 11:34

bsardo assigned VeronikaSolovei9 and bsardo Jan 25, 2024

VeronikaSolovei9 reviewed Jan 28, 2024

View reviewed changes

SyntaxNode reviewed Jan 30, 2024

View reviewed changes

Merge master branch

423a30f

ashishshinde-pubm requested review from VeronikaSolovei9 and SyntaxNode February 2, 2024 04:05

bsardo assigned guscarreon and unassigned bsardo Feb 8, 2024

bsardo assigned hhhjort and unassigned guscarreon Feb 26, 2024

bsardo assigned bsardo and unassigned hhhjort Apr 8, 2024

Merge branch 'master'

9450e40

fix unit tests

6cd54de

ashishshinde-pubm added 2 commits July 15, 2024 17:28

Merge branch 'master' of github.com:ashishshinde-pubm/prebid-server i…

2dac953

…nto SNBR_HookStage

fix UTs

dbaff7e

bsardo reviewed Oct 7, 2024

View reviewed changes

Pubmatic-Dhruv-Sonone added 3 commits October 18, 2024 10:13

Merging latest master

35f2d02

Fix merge conflicts

e62b33a

Fixing UT

33bf0e5

SyntaxNode added the do not port label Nov 5, 2024

Merge remote-tracking branch 'upstream/master' into SNBR_HookStage

fd14752

SyntaxNode reviewed Nov 15, 2024

View reviewed changes

Reverting the usage of jsonutil

dfc26e7

bsardo assigned hhhjort and unassigned VeronikaSolovei9 Dec 9, 2024

hhhjort reviewed Dec 12, 2024

View reviewed changes

Pubmatic-Dhruv-Sonone added 2 commits December 22, 2024 17:54

Fixing typo

f952f90

Using foundErrors flag instead of ao.Response nil check

956c51e

		@@ -0,0 +1,80 @@
		package openrtb_ext

		import "github.com/prebid/openrtb/v19/openrtb2"

		ao.HookExecutionOutcome = stageOutcomes
		err := setSeatNonBidRaw(ao.RequestWrapper, response, ao.SeatNonBid)

Introduce SeatNonBid in hookoutcome #3416

Are you sure you want to change the base?

Introduce SeatNonBid in hookoutcome #3416

Conversation

ashishshinde-pubm commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashishshinde-pubm commented Jan 31, 2024 • edited Loading

VeronikaSolovei9 commented Feb 6, 2024

ashishshinde-pubm commented Feb 19, 2024

bsardo commented Feb 22, 2024

hhhjort commented Mar 12, 2024

zhongshixi commented Apr 15, 2024 • edited Loading

ashishshinde-pubm commented Apr 18, 2024

zhongshixi commented Apr 19, 2024

zhongshixi commented Apr 22, 2024 • edited Loading

ashishshinde-pubm commented Apr 22, 2024

ashishshinde-pubm commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pubmatic-Dhruv-Sonone commented Oct 21, 2024

bsardo commented Nov 4, 2024

Pubmatic-Dhruv-Sonone commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pubmatic-Dhruv-Sonone commented Dec 25, 2024

ashishshinde-pubm commented Jan 31, 2024 •

edited

Loading

zhongshixi commented Apr 15, 2024 •

edited

Loading

zhongshixi commented Apr 22, 2024 •

edited

Loading