Single stored auction response #2132

VeronikaSolovei9 · 2022-01-13T06:25:33Z

Issue: #861
Implementation of Single Stored Auction Response

guscarreon · 2022-02-15T17:57:32Z

endpoints/openrtb2/auction.go

+	// - there is only stored request id in incoming request
+	// - there is at least one stored imp with stored auction response
+	// Need to update ImpExtPrebid after stored responses/impressions applied to incoming request
+	impInfo, errs := parseImpInfo(requestJson)


Instead of calling parseImpInfo(requestJson) again here, can we pass the impInfo calculated in line 289 just as we do in the processStoredRequests call of line 295?

Right, this is how it was before I talked to Bret about we need to support stored responses in stored requests. Imp ext can be modified by applying store request and this stored request may have stored response in it. This is why we call processStoredRequests first and processStoredAuctionResponses then. I added a comment above with explanations why we need to call it here again (L1572-1576)

guscarreon · 2022-02-15T21:33:07Z

endpoints/openrtb2/amp_auction.go

@@ -316,27 +318,27 @@ func (deps *endpointDeps) parseAmpRequest(httpRequest *http.Request) (req *openr
 	}

 	// At this point, we should have a valid request that definitely has Targeting and Cache turned on
-	e = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: req}, true)
+	e = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: req}, true, false)


I'm curious. Why are we using false and not evaluating whether or not we have elements in the storedAuctionResponses array as we do in endpoints/openrtb2/auction.go?

321 - e = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: req}, true, false) 321 + e = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: req}, true, len(storedAuctionResponses) > 0)

You are right, fixed, thank you!

guscarreon · 2022-02-15T21:35:32Z

endpoints/openrtb2/video_auction.go

@@ -249,7 +249,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re
 	// Populate any "missing" OpenRTB fields with info from other sources, (e.g. HTTP request headers).
 	deps.setFieldsImplicitly(r, bidReq) // move after merge

-	errL = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: bidReq}, false)
+	errL = deps.validateRequest(&openrtb_ext.RequestWrapper{BidRequest: bidReq}, false, false)


I see we don't parse the storedAuctionResponses in this endpoint as compared with the others? Does that feature don't apply to video?

There is no way to pass custom imp.ext for video endpoint at this time. We don't support storedResponses for video requests.

guscarreon · 2022-02-15T21:49:03Z

exchange/exchange.go

+
+	} else {
+		adapterBids, adapterExtra, anyBidsReturned = e.getAllBids(auctionCtx, bidderRequests, bidAdjustmentFactors, conversions, accountDebugAllow, r.GlobalPrivacyControlHeader, debugLog.DebugOverride)
+	}


Function call liveAdapters := listBiddersWithRequests(bidderRequests) would go to waste if check in line 220 evaluates to true. Given that liveAdapters will be used downstream, can we optimize a bit by calling listBiddersWithRequests(bidderRequests) only if needed?

205 - // List of bidders we have requests for. 206 - liveAdapters := listBiddersWithRequests(bidderRequests) 207 208 // If we need to cache bids, then it will take some time to call prebid cache. 209 // We should reduce the amount of time the bidders have, to compensate. 210 auctionCtx, cancel := e.makeAuctionContext(ctx, cacheInstructions.cacheBids) 211 defer cancel() 212 213 // Get currency rates conversions for the auction 214 conversions := e.getAuctionCurrencyRates(requestExt.Prebid.CurrencyConversions) 215 216 var adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid 217 var adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra 218 var anyBidsReturned bool + var liveAdapters []openrtb_ext.BidderName 219 220 if len(r.StoredAuctionResponses) > 0 { 221 adapterBids, liveAdapters, err = buildStoreAuctionResponse(r.StoredAuctionResponses) 222 if err != nil { 223 return nil, err 224 } 225 anyBidsReturned = true 226 227 } else { + liveAdapters = listBiddersWithRequests(bidderRequests) 228 adapterBids, adapterExtra, anyBidsReturned = e.getAllBids(auctionCtx, bidderRequests, bidAdjustmentFactors, conversions, accountDebugAllow, r.GlobalPrivacyControlHeader, debugLog.DebugOverride) 229 } exchange/exchange.go

Agree, moved

guscarreon · 2022-02-15T21:57:25Z

exchange/exchange_test.go

@@ -3667,6 +3667,190 @@ func TestFPD(t *testing.T) {

 }

+func TestStoredAuctionResponses(t *testing.T) {


I agree with TestStoredAuctionResponses because it calls HoldAuction but, can we also create a unit test to assert buildStoreAuctionResponse by its own? I don't have a problem if we keep both test cases. I'd like to hear @bsardo's take on this though.

func buildStoreAuctionResponse has 100% coverage (except of unmarshal json error handler).
TestStoredAuctionResponses is mostly written to test buildStoreAuctionResponse, like you said separate test will be sort of copy of existing one.
Also this is a private function.
I'm ok to add extra test for it, will do what you think is better.

Hm it's kind of a matter of personal preference. The coverage is there. I agree private functions generally don't need their own dedicated unit test if the function is simplistic.
With that said, I would lean towards writing a unit test for this function given that there's a bit going on there and simplifying this test so it just focuses on exercising the conditional if len(r.StoredAuctionResponses) > 0 { in exchange.go but I understand if we don't want to do that at this point :)

bsardo · 2022-01-24T16:30:41Z

endpoints/openrtb2/auction.go

+	if len(storedAuctionResponseIds) > 0 {
+		storedAuctionResponses, errs := deps.storedRespFetcher.FetchResponses(ctx, storedAuctionResponseIds)
+		if len(errs) > 0 {
+			return nil, errs
+		}
+		for impId, respId := range impIdToRespId {
+			impIdToStoredResp[impId] = storedAuctionResponses[respId]
+		}
+		return impIdToStoredResp, nil
+	}
+	return nil, nil


Does it make sense to fetch responses here? What if this function just produced a map of imp ids to stored auction response ids leaving the stored auction response fetching and response building near where bid response fetching and and response building typically happen in HoldAuction where we call getAllBids?
It seems a little less than ideal to me to fetch any kind of response before we've finished parsing and validating the request. Thoughts?

To fetch stored responses in getAllBids we will need to pass fetcher all the way down the stack. We don't have this option now.
Also processStoredAuctionResponses function is a part of parseRequest where previous function is processStoredRequests. To me it makes sense to have it all in one place and fetch extra data sooner (and error out in case of errors) than later.
Fetching bids is an http calls, not fetcher calls, it would be confusing to mix this logic. In this case we don't even go that deep, we don't call getAllBids if there were stored responses.

bsardo · 2022-02-15T18:00:32Z

endpoints/openrtb2/auction.go

@@ -315,7 +322,7 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request) (req *openrtb_

 	lmt.ModifyForIOS(req.BidRequest)

-	errL := deps.validateRequest(req, false)
+	errL := deps.validateRequest(req, false, len(storedAuctionResponses) > 0)


Nitpick: can we put this conditional on its own line? It makes it a bit easier to read IMO and it will help us ensure test coverage when using the coverage tooling.

Do you mean to extract it to variable? Like hasStoredResponses := len(storedAuctionResponses) > 0?

If yes - done :)

bsardo · 2022-02-16T15:29:38Z

endpoints/openrtb2/auction.go

+	// Input request may have stored request or stored imps that will modify imps from incoming request
+	// Cases:
+	// - there is only stored request id in incoming request
+	// - there is at least one stored imp with stored auction response
+	// Need to update ImpExtPrebid after stored responses/impressions applied to incoming request


Can we modify this comment slightly and perhaps move it just above the method signature?
Maybe something like:
processStoredAuctionResponses takes the incoming request as JSON with any stored requests/imps already merged into it, scans it to find any stored auction response ids in the request/imps and produces a map of imp IDs to stored auction responses. 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.

bsardo · 2022-02-16T15:56:23Z

endpoints/openrtb2/auction.go

+			impId, err := jsonparser.GetString(impData.Imp, "id")
+			if err != nil {
+				return nil, []error{err}
+			}
+
+			impIdToRespId[impId] = impData.ImpExtPrebid.StoredAuctionResponse.ID


This logic seems like it could result in some unexpected behavior if the imp IDs are not unique.
Suppose the request has three imps each mapping to the following stored auction responses:
imp-id-1 --> resp-1
imp-id-2 --> resp-2
imp-id-1 --> resp-3
It appears the two imps with id imp-id-1 will have resp-3.
I don't think we perform imp uniqueness validation, however, this scenario could produce unexpected results. Do we need to handle this case?

Imp ids must be unique per request, the situation you described is not possible.
It can be the situation where 2 or more imps have same stored response:
imp-id-1 --> resp-1
imp-id-2 --> resp-1
but in this case fetcher handles this properly.

Gotcha. I see that imp IDs are guaranteed to be unique at this point because we ensured this during validation in endpoints/openrtb2/auction.go#validateRequest.

bsardo · 2022-02-16T16:33:42Z

exchange/exchange.go

@@ -1076,3 +1093,39 @@ func listBiddersWithRequests(bidderRequests []BidderRequest) []openrtb_ext.Bidde

 	return liveAdapters
 }
+
+func buildStoreAuctionResponse(storedActionResponses map[string]json.RawMessage) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []openrtb_ext.BidderName, error) {


Nitpick: typo - I think storedActionResponses should be storedAuctionResponses?

Yes, renamed

@VeronikaSolovei9 I see you modified the function name, which looks good, but I was referring to the first parameter storedActionResponses which should be storedAuctionResponses.

bsardo · 2022-02-16T17:39:38Z

endpoints/openrtb2/auction.go

+
+		if impData.ImpExtPrebid.StoredAuctionResponse != nil {
+			if len(impData.ImpExtPrebid.StoredAuctionResponse.ID) == 0 {
+				return nil, []error{fmt.Errorf("request.imp[%d] has ext.prebid.storedauctionresponse specified, but \"id\" field is missing ", index)}


Can we add test coverage for this error case, perhaps to TestParseRequestStoredResponses?

Added TestProcessStoredAuctionResponsesErrors

bsardo · 2022-02-16T17:40:09Z

endpoints/openrtb2/auction.go

+			storedAuctionResponseIds = append(storedAuctionResponseIds, impData.ImpExtPrebid.StoredAuctionResponse.ID)
+
+			impId, err := jsonparser.GetString(impData.Imp, "id")
+			if err != nil {


Can we add test coverage for this error case, perhaps to TestParseRequestStoredResponses?

Added TestProcessStoredAuctionResponsesErrors

bsardo · 2022-02-16T17:43:29Z

endpoints/openrtb2/auction_test.go

+
+	testCases := []struct {
+		description         string
+		givenIsAmp          bool


I think you can simplify this a bit by removing givenIsAmp and hasStoredResponses and just hardcoding them in the function call since the values are the same for all test cases.

Right, modified.

bsardo · 2022-02-16T18:01:19Z

exchange/exchange_test.go

@@ -3667,6 +3667,190 @@ func TestFPD(t *testing.T) {

 }

+func TestStoredAuctionResponses(t *testing.T) {


Hm it's kind of a matter of personal preference. The coverage is there. I agree private functions generally don't need their own dedicated unit test if the function is simplistic.
With that said, I would lean towards writing a unit test for this function given that there's a bit going on there and simplifying this test so it just focuses on exercising the conditional if len(r.StoredAuctionResponses) > 0 { in exchange.go but I understand if we don't want to do that at this point :)

VeronikaSolovei9 · 2022-02-17T06:55:50Z

@guscarreon @bsardo guys, test for func buildStoredAuctionResponse turns into copy of TestStoredAuctionResponses, where instead of HoldAuction we call buildStoredAuctionResponse. I can push the duplicate, but idea is the same. Or should I modify it to have just one test for buildStoredAuctionResponse?

bsardo · 2022-02-22T15:09:52Z

@guscarreon @bsardo guys, test for func buildStoredAuctionResponse turns into copy of TestStoredAuctionResponses, where instead of HoldAuction we call buildStoredAuctionResponse. I can push the duplicate, but idea is the same. Or should I modify it to have just one test for buildStoredAuctionResponse?

@VeronikaSolovei9 I don't think you need to duplicate much. What you have is good. My suggestion though we be to do something like this:

Take your existing TestStoredAuctionResponses test and make it a unit test for buildStoredAuctionResponse (call it TestBuildStoredAuctionResponse) by adding two more expected output parameters (e.g. seatBids and bidderNames). Then make the body of your test as simple as this: for _, test := range testCases { seatBids, bidderNames, err := buildStoredAuctionResponse(test.in.StoredAuctionResponses) assert.Equal(t, test.expected.seatBids, seatBids, test.desc) assert.Equal(t, test.expected.bidderNames, bidderNames, test.desc) assert.NoErrorf(t, err, "%s. HoldAuction error: %v \n", test.desc, err) }
Create a second test that just has a single test case with a single stored auction response so it exercises the line if len(r.StoredAuctionResponses) > 0 { in exchange.go. You can take most of what you remove from TestStoredAuctionResponses to setup the mock bid request and the exchange here.

I'm not adamant about making this change, however, I think it would help us better understand buildStoredAuctionResponse.

bsardo

LGTM

guscarreon

LGTM

mansinahar requested review from bsardo and guscarreon January 13, 2022 18:26

mansinahar assigned bsardo and guscarreon Jan 13, 2022

VeronikaSolovei9 force-pushed the single_stored_response branch 2 times, most recently from d17d316 to 68c1a5c Compare February 7, 2022 09:48

VeronikaSolovei9 marked this pull request as ready for review February 9, 2022 06:25

VeronikaSolovei9 mentioned this pull request Feb 16, 2022

Stored bid response #2160

Closed

guscarreon reviewed Feb 16, 2022

View reviewed changes

VeronikaSolovei9 force-pushed the single_stored_response branch from e0a4a77 to ad78a4f Compare February 16, 2022 08:44

bsardo reviewed Feb 16, 2022

View reviewed changes

VeronikaSolovei9 added 8 commits February 22, 2022 12:25

Stored auction response

e476915

Unit test fixes

00fcee6

Added stored auction resp support to AMP endpoint

18b5580

Unit tests

0c0ce02

Unit tests

018560a

Code clean up

39f1c47

Code review fixes

943651a

Unit test refactoring

88d0b18

VeronikaSolovei9 force-pushed the single_stored_response branch from a70092d to 88d0b18 Compare February 22, 2022 20:26

bsardo previously approved these changes Feb 23, 2022

View reviewed changes

Unit test fix

8462176

VeronikaSolovei9 dismissed bsardo’s stale review via 8462176 February 23, 2022 19:31

bsardo previously approved these changes Feb 25, 2022

View reviewed changes

Code clean up

5f86a4c

VeronikaSolovei9 dismissed bsardo’s stale review via 5f86a4c February 28, 2022 23:53

guscarreon approved these changes Mar 1, 2022

View reviewed changes

VeronikaSolovei9 merged commit 8966116 into master Mar 1, 2022

pm-nilesh-chate added a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Mar 30, 2022

Single stored auction response (prebid#2132)

823cba6

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

Single stored auction response (prebid#2132)

2c0ebc4

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Single stored auction response (prebid#2132)

7dc1861

SyntaxNode deleted the single_stored_response branch March 13, 2023 13:55

		@@ -3667,6 +3667,190 @@ func TestFPD(t *testing.T) {

		}

		func TestStoredAuctionResponses(t *testing.T) {

Single stored auction response #2132

Single stored auction response #2132

Conversation

VeronikaSolovei9 commented Jan 13, 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

VeronikaSolovei9 Feb 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

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 Feb 17, 2022 • edited Loading

bsardo commented Feb 22, 2022 • edited Loading

bsardo left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jan 13, 2022 •

edited

Loading

VeronikaSolovei9 Feb 16, 2022 •

edited

Loading

VeronikaSolovei9 commented Feb 17, 2022 •

edited

Loading

bsardo commented Feb 22, 2022 •

edited

Loading