Stored bid response #2160

VeronikaSolovei9 · 2022-02-16T03:38:39Z

Stored bid responses implementation.
-Depends on stored auction response branch, PR #2132
-No unit tests

Code flow:
auction.go -> func parseRequest: added functionality to extract all stored bid responses and loads them from data store.
returns stored bid responses: imp id to bidder to stored response body
impBidderToStoredBidResponse := make(map[string]map[string]json.RawMessage)
if request is valid, data successfully loaded - sets StoredBidResponses: storedBidResponses to AuctionRequest and passes it to HoldAuction

utils.go -> func cleanOpenRTBRequests: builds list of Bidder Requests. In order to support stored bid responses, where in the same request impressions may have real bidder data and others may have stored bid responses I added BidderStoredResponses map[string]json.RawMessage to BidderRequest. One BidderRequest may have both real request and/or BidderStoredResponses initialized.

exchange/bidder.go -> func requestBid: executes real and fake requests to adapters
case if len(bidderRequest.BidRequest.Imp) > 0 { handles real request fetching, executes MakeRequests, sends real http requests
case if len(bidderRequest.BidderStoredResponses) > 0 { handles stored bid responses, doesn't execute MakeRequests
both of these cases sets response data to channel responseChannel. Stored bid response in this case looks like real bidder response, so it goes through usual processing including exercising bidder related code - func MakeBids

VeronikaSolovei9 · 2022-03-01T08:24:24Z

Merged dependent branch for single stored response and rebased on top of latest upstream

AlexBVolcy · 2022-03-03T02:25:04Z

exchange/exchange.go

@@ -218,7 +221,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 	var liveAdapters []openrtb_ext.BidderName

 	if len(r.StoredAuctionResponses) > 0 {
-		adapterBids, liveAdapters, err = buildStoredAuctionResponse(r.StoredAuctionResponses)
+		adapterBids, liveAdapters, err = buildStoreAuctionResponse(r.StoredAuctionResponses)


This might be a spelling error, do we want this to say buildStoredAuctionResponse instead of buildStoreAuctionResponse

Renamed. It was fixed in Stored Single Response PR, but I rebased it incorrectly. Good catch!

AlexBVolcy · 2022-03-03T02:25:34Z

exchange/exchange.go


 	adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, 0)
 	liveAdapters := make([]openrtb_ext.BidderName, 0)
-	for impId, storedResp := range storedAuctionResponses {
+	for impId, storedResp := range storedActionResponses {


Also wondering if this is a spelling error where you have the word Action instead of Auction

AlexBVolcy · 2022-03-03T02:25:58Z

exchange/exchange_test.go

@@ -3885,7 +3885,7 @@ func TestBuildStoredAuctionResponses(t *testing.T) {
 	}
 	for _, test := range testCases {

-		bids, adapters, err := buildStoredAuctionResponse(test.in.StoredAuctionResponses)
+		bids, adapters, err := buildStoreAuctionResponse(test.in.StoredAuctionResponses)


Same question about potential spelling error Store vs. Stored

AlexBVolcy · 2022-03-03T02:27:55Z

exchange/utils.go

+	if len(req.StoredBidResponses) > 0 {
+		// delete imps with stored bid resp
+		imps := req.BidRequest.Imp
+		req.BidRequest.Imp = nil //or new slice?


Is this comment //or new slice? still needed?

Removed, no need of this comment

AlexBVolcy · 2022-03-03T02:28:22Z

exchange/utils.go

+			for bidderName, storedResp := range storedData {
+				if _, ok := bidderToBidderResponse[openrtb_ext.BidderName(bidderName)]; !ok {
+					//new bidder with stored bid responses
+					//!!! check if bidder is valid/exists


Is this comment still needed, or if it is, could those !!! be removed?

Removed and added this validation to 'processStoredAuctionResponses'

AlexBVolcy

I had a few initial comments related to spelling and comments. Could you update the PR's description, to better describe what this PR is trying to accomplish? Is this trying to accomplish the issue outlined here #861

VeronikaSolovei9 · 2022-03-03T09:56:13Z

Good comments, addressed all of them, added description of changes to PR

AlexBVolcy

So I tested code coverage for this PR, and I left comments for which new code additions aren't yet covered by tests. Could you update this?

AlexBVolcy · 2022-03-15T16:01:45Z

endpoints/openrtb2/auction.go

+				if _, isValid := deps.bidderMap[bidderResp.Bidder]; !isValid {
+					return nil, nil, []error{fmt.Errorf("request.imp[impId: %s].ext contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impId, bidderResp.Bidder)}
+				}
+				//assuming bidder is unique per one bid stored response


Is this comment necessary? Just checking :)

I'm 90% sure this should be unique, but I need to check with Bret. I'll remove the comment when I get the answer.
Discussion: #861 (comment)

Per discussion with Bret we don't need to support this. I'll remove a comment on my next commit.

AlexBVolcy · 2022-03-15T16:08:10Z

exchange/utils.go

@@ -66,6 +70,12 @@ func cleanOpenRTBRequests(ctx context.Context,
 	allBidderRequests, errs = getAuctionBidderRequests(req, requestExt, bidderToSyncerKey, impsByBidder, aliases)

 	if len(allBidderRequests) == 0 {
+		if len(bidderToBidderResponse) > 0 {
+			//all imps have stored bid responses


Could you add test coverage for this block?

AlexBVolcy · 2022-03-15T16:08:43Z

exchange/utils.go

@@ -150,10 +159,25 @@ func cleanOpenRTBRequests(ctx context.Context,

 		if bidRequestAllowed {
 			privacyEnforcement.Apply(bidderRequest.BidRequest)
+
+			if bidderWithStoredBidResponses, ok := bidderToBidderResponse[bidderRequest.BidderName]; ok {
+				//this bidder has real imps and imps with stored bid response


I think the above if statement explains the comment here. So i think this could be removed!

Also, this code block from lines 164-177, aren't covered by tests.

AlexBVolcy · 2022-03-15T16:23:56Z

endpoints/openrtb2/auction.go

 			impIdToRespId[impId] = impData.ImpExtPrebid.StoredAuctionResponse.ID

 		}
+		if len(impData.ImpExtPrebid.StoredBidResponse) > 0 {
+
+			bidderStoredRespId := make(map[string]string)


Could add tests for these new changes (lines 1620-1633), I ran a code coverage test and see these aren't covered.

AlexBVolcy · 2022-03-15T16:24:19Z

endpoints/openrtb2/auction.go

 	impInfo, errs := parseImpInfo(requestJson)
 	if len(errs) > 0 {
-		return nil, errs
+		return nil, nil, errs


Could you a test case that covers this error case?

AlexBVolcy · 2022-03-15T16:24:46Z

endpoints/openrtb2/auction.go

 	}
 	if len(storedAuctionResponseIds) > 0 {
 		storedAuctionResponses, errs := deps.storedRespFetcher.FetchResponses(ctx, storedAuctionResponseIds)
 		if len(errs) > 0 {
-			return nil, errs
+			return nil, nil, errs


Could you add test coverage for this error case

AlexBVolcy · 2022-03-15T16:26:11Z

endpoints/openrtb2/auction.go

 		}
 		for impId, respId := range impIdToRespId {
 			impIdToStoredResp[impId] = storedAuctionResponses[respId]
 		}
-		return impIdToStoredResp, nil
+


Could you add test coverage for these additions (Lines 1646-1652)

AlexBVolcy · 2022-03-15T16:29:54Z

exchange/bidder.go

-			go func(data *adapters.RequestData) {
-				responseChannel <- bidder.doRequest(ctx, data)
-			}(oneReqData) // Method arg avoids a race condition on oneReqData
+	if len(bidderRequest.BidderStoredResponses) > 0 {


Could you add test coverage for this code block (177-198)

AlexBVolcy · 2022-03-15T16:30:28Z

exchange/bidder.go

@@ -243,6 +276,15 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb2.B
 					}
 				}

+				if len(bidderRequest.BidderStoredResponses) > 0 {


Could you add test coverage for this code block (line 279-287)

AlexBVolcy · 2022-03-15T16:32:57Z

exchange/utils.go

@@ -741,3 +765,40 @@ func applyFPD(fpd *firstpartydata.ResolvedFirstPartyData, bidReq *openrtb2.BidRe
 		bidReq.User = fpd.User
 	}
 }
+
+func buildBidderRequestsWithBidResponses(req AuctionRequest, aliases map[string]string) map[openrtb_ext.BidderName]BidderRequest {


Could you add tests for this function

VeronikaSolovei9 · 2022-03-16T06:23:15Z

When unit tests are added I'll make sure it's all covered.

bsardo · 2022-03-16T14:55:32Z

endpoints/openrtb2/auction.go

@@ -1582,44 +1583,76 @@ func getJsonSyntaxError(testJSON []byte) (bool, string) {
 // Note that processStoredAuctionResponses must be called after processStoredRequests
 // because stored imps and stored requests can contain stored auction responses
 // so the stored requests/imps have to be merged into the incoming request prior to processing stored auction responses.
-func (deps *endpointDeps) processStoredAuctionResponses(ctx context.Context, requestJson []byte) (map[string]json.RawMessage, []error) {
+func (deps *endpointDeps) processStoredAuctionResponses(ctx context.Context, requestJson []byte) (map[string]json.RawMessage, map[string]map[string]json.RawMessage, []error) {


This function is doing a bit too much IMO. Perhaps we can split this into a couple of functions each with a single responsibility?

This function extracts storedResponses and storedBidResponses from raw request. We use the same fetcher for both of them. Because of parsing a raw request we extract all ids, get stored data and put this data to useful format for future use: return impIdToStoredResp, impBidderToStoredBidResponse, nil. What do you think would be a good split?

bsardo · 2022-03-16T15:09:29Z

exchange/utils.go

-	impsByBidder, err := splitImps(req.BidRequest.Imp)
-	if err != nil {
-		errs = []error{err}
+	aliases, errs := parseAliases(req.BidRequest)
+	if len(errs) > 0 {
 		return
 	}


Are there any implications reversing the order in which aliases and imps are processed? I imagine it is just a matter of which error is reported if both splitting imps and parsing aliases result in errors.

I just moved it to the first statement of the cleanOpenRTBRequests function, because aliases are needed in buildBidderRequestsWithBidResponses. buildBidderRequestsWithBidResponses should be executed before everything else, because it modifies request - removes all imps with stored bid responses from imp array.

aliases, errs := parseAliases(req.BidRequest) if len(errs) > 0 { return } allowedBidderRequests = make([]BidderRequest, 0, 0) bidderToBidderResponse := buildBidderRequestsWithBidResponses(req, aliases) impsByBidder, err := splitImps(req.BidRequest.Imp) ...

bsardo · 2022-03-16T15:13:48Z

exchange/utils.go

+	bidderToBidderResponse := buildBidderRequestsWithBidResponses(req, aliases)
+
+	impsByBidder, err := splitImps(req.BidRequest.Imp)


You're building bidderToBidderResponse before splitting imps because it is easier to process stored bid responses before splitting right?

buildBidderRequestsWithBidResponses function removes "fake" imps from request. Split imps works with "real" impressions only. Please my previous comment - it has more context on this.

bsardo · 2022-03-16T15:26:12Z

exchange/utils.go

+
+func buildBidderRequestsWithBidResponses(req AuctionRequest, aliases map[string]string) map[openrtb_ext.BidderName]BidderRequest {
+	var bidderToBidderResponse map[openrtb_ext.BidderName]BidderRequest
+	if len(req.StoredBidResponses) > 0 {


This function looks good. Can we flatten it a bit by using an early return here?

if len(req.StoredBidResponses) <= 0 { return bidderToBidderResponse }

bsardo · 2022-03-16T15:38:38Z

exchange/utils.go

+		// delete imps with stored bid resp
+		imps := req.BidRequest.Imp
+		req.BidRequest.Imp = nil
+		for _, imp := range imps {
+			if _, ok := req.StoredBidResponses[imp.ID]; !ok {
+				//add real imp back to request
+				req.BidRequest.Imp = append(req.BidRequest.Imp, imp)
+			}
+		}


On second thought, after looking at the calling code, I feel like this logic in this function should probably be split up a bit into separate functions so that we can appropriately name the different steps involved.
I was particularly confused by this code that follows the call to buildBidderRequestsWithBidResponses:

if len(allBidderRequests) == 0 { if len(bidderToBidderResponse) > 0 {

This leads me to think that it would benefit the reader if the part of this function that deletes all imps and then re-adds those that don't have a stored bid response were in its own function with some name like Strip/RemoveImpsWithStoredResponses.
The following call could be to the rest of this function that just focuses on building the bidder to stored responses map.
Both of these could be called in succession prior to executing that nested conditional I referenced here.

This kind of refactor should also reduce functional complexity resulting in simpler tests.

Perhaps we should have a stored responses package so its core logic is all in one place. Something like:
BuildStoredBidResponses --> returns the map
ExtractStoredBidResponse --> removes imps from the bid request that have a stored bid response
Prepare/SendStoredResponses --> prepares a request with its stored response to be sent on the channel
I might have missed some other functions here that are needed. This is just off the top of my head.

bsardo · 2022-03-16T19:32:37Z

exchange/bidder.go

+		//if stored bid responses are present - replace impIds and add them as is to responseChannel <- stored responses
+		if responseChannel == nil {
+			dataLen = dataLen + len(bidderRequest.BidderStoredResponses)
+			responseChannel = make(chan *httpCallInfo, dataLen)
 		}
+		for impId, bidresp := range bidderRequest.BidderStoredResponses {
+			//always one element in reqData because stored response is mapped to single imp
+			reqDataForStoredResp := adapters.RequestData{
+				Method: "POST",
+				Uri:    "",
+				Body:   []byte(impId), //use it to pass imp id for stored resp
+			}
+			respData := &httpCallInfo{
+				request: &reqDataForStoredResp,
+				response: &adapters.ResponseData{
+					StatusCode: 200,
+					Body:       bidresp,
+				},
+				err: nil,
+			}
+			go func() { responseChannel <- respData }()
+		}
+


This seems like another possible function that can be extracted into a stored responses package to reduce some of the complexity here.

VeronikaSolovei9 force-pushed the stored_bid_response branch from 6536ed6 to d246690 Compare February 16, 2022 08:01

bsardo requested review from bsardo and AlexBVolcy February 28, 2022 18:12

bsardo assigned bsardo and AlexBVolcy Feb 28, 2022

VeronikaSolovei9 force-pushed the stored_bid_response branch 2 times, most recently from 1030a5b to 193e126 Compare March 1, 2022 08:23

VeronikaSolovei9 marked this pull request as ready for review March 2, 2022 06:49

AlexBVolcy reviewed Mar 3, 2022

View reviewed changes

AlexBVolcy reviewed Mar 15, 2022

View reviewed changes

bsardo reviewed Mar 16, 2022

View reviewed changes

VeronikaSolovei9 closed this Mar 17, 2022

VeronikaSolovei9 force-pushed the stored_bid_response branch from d164d96 to 5fa462b Compare March 17, 2022 19:17

VeronikaSolovei9 mentioned this pull request Mar 17, 2022

Stored bid responses #2188

Merged

		bidderToBidderResponse := buildBidderRequestsWithBidResponses(req, aliases)

		impsByBidder, err := splitImps(req.BidRequest.Imp)

Stored bid response #2160

Stored bid response #2160

Conversation

VeronikaSolovei9 commented Feb 16, 2022 • edited Loading

VeronikaSolovei9 commented Mar 1, 2022

Choose a reason for hiding this comment

VeronikaSolovei9 Mar 3, 2022 • edited Loading

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

AlexBVolcy left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Mar 3, 2022

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Mar 16, 2022 • edited Loading

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 commented Mar 16, 2022

Choose a reason for hiding this comment

VeronikaSolovei9 Mar 17, 2022 • edited Loading

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 commented Feb 16, 2022 •

edited

Loading

VeronikaSolovei9 Mar 3, 2022 •

edited

Loading

VeronikaSolovei9 Mar 16, 2022 •

edited

Loading

VeronikaSolovei9 Mar 17, 2022 •

edited

Loading