Mirror stored imp data to bid response #1889

VeronikaSolovei9 · 2021-06-15T08:50:13Z

Implements #1075

endpoints/openrtb2/auction.go

exchange/exchange.go

endpoints/openrtb2/auction.go

exchange/exchange.go

endpoints/openrtb2/auction.go

exchange/exchange.go

endpoints/openrtb2/auction.go

SyntaxNode · 2021-06-29T17:09:23Z

endpoints/openrtb2/auction.go

+		}
+
+		if includeStoredImps && offset != -1 {
+			impId, err := jsonparser.GetString(resolvedImp, "id")


A Note On jsonparser:
This is substantially faster for reading values from a json blob than the Go json package, but you need to keep in mind that each jsonparser.GetXXX call re-parses the entire json. Based on performance measurements, the tipping point of efficiency is around 4 calls. At that point, please consider switching to EachKey to use a single pass.

Good point. We are about to reach 4 calls.
The very last call takes storedImps[impIds[i]] as input, 3 others resolvedImp
I added a comment about this.

SyntaxNode · 2021-06-29T17:14:09Z

endpoints/openrtb2/auction.go

+			videoData, _, _, err := jsonparser.Get(storedImps[impIds[i]], "video")
+			if err == nil {
+				impToStoredReq[impId] = videoData
+			}


To improve performance for non-video ads, please nest the other jsonparser checks under includeStoredImps. ie:

includeVideoAttributes, err := jsonparser.GetBoolean(resolvedImp, "ext", "prebid", "options", "echovideoattrs") if err != nil && err != jsonparser.KeyPathNotFoundError { return nil, nil, []error{err} } if includeVideoAttributes { _, _, videoImpOffset, err := jsonparser.Get(resolvedImp, "video") if err != nil && err != jsonparser.KeyPathNotFoundError { return nil, nil, []error{err} } if videoImpOffset != -1 { impId, err := jsonparser.GetString(resolvedImp, "id") if err != nil { return nil, nil, []error{err} } //extract video attributes only videoData, _, _, err := jsonparser.Get(storedImps[impIds[i]], "video") if err == nil { impToStoredReq[impId] = videoData } } }

Yes, modified

SyntaxNode · 2021-06-29T17:15:09Z

endpoints/openrtb2/auction.go

+			//extract video attributes only
+			videoData, _, _, err := jsonparser.Get(storedImps[impIds[i]], "video")
+			if err == nil {
+				impToStoredReq[impId] = videoData


The name impToStoredReq is now a bit misleading since it's a subset for video auctions. Perhaps it would be best to include the full stored request here and "cut out" the video section to bid response handler?

I went through this again. The reason I put this here is to optimize amount of data we have in memory (have only video impressions we will use later) and to reduce complexity of code execution in insertStoredImpData function: if no stored video data found - do not execute any logic. Also if we leave this code here - it will fit the logic around it: prepare stored request/video data.
I'm open to modify this and move this piece of code to bid response handler (insertStoredImpData). Please advice.

Renamed impToStoredReq to impToStoredVideoReq

Modified according to discussion

endpoints/openrtb2/auction_test.go

util/jsonutil/jsonutil_test.go

mansinahar · 2021-07-06T10:22:43Z

endpoints/openrtb2/auction.go

@@ -1391,22 +1394,52 @@ func (deps *endpointDeps) processStoredRequests(ctx context.Context, requestJson
 					err = fmt.Errorf("imp.ext.prebid.storedrequest.id %s: Stored Imp has Invalid JSON: %s", impIds[i], Err)
 				}
 			}
-			return nil, []error{err}
+			return nil, impToStoredVideoReq, []error{err}


Why not return nil here instead of impToStoredVideoReq? I know that this map will be empty here but an empty map is not equal to nil and so any potential nil checks on this map outside of this function would fail and also from a reading perspective, nil clearly indicates that a nil map is returned vs impToStoredVideoReq indicates that there might be some key-values in this map

Agreed, modified

mansinahar · 2021-07-06T10:58:40Z

endpoints/openrtb2/auction.go

+					return nil, nil, []error{err}
+				}
+				//extract video attributes only
+				videoData, _, _, err := jsonparser.Get(storedImps[impIds[i]], "video")


How about including the entire stored request here rather than just the video stored req part? You can then check for the includeVideoAttributes flag on response building and if it is set, extract the video part and include that in the response ext. This way the entire stored request would be present in the Auction object throughout the auction process, in case anything else needs it in the future. Also, doing something response specific in processStoredRequests just doesn't seem right

Modified according to discussion

VeronikaSolovei9 · 2021-07-23T04:01:41Z

@mansinahar @SyntaxNode code modified according to our code review discussion, please re-review

mansinahar · 2021-07-28T00:23:30Z

exchange/exchange.go

@@ -135,6 +136,11 @@ func NewExchange(adapters map[openrtb_ext.BidderName]adaptedBidder, cache prebid
 	}
 }

+type StoredImpData struct {


We should probably call this something along the lines of ImpExtInfo as the thought here is that this struct would be able to hold imp related extra info like the echoVideoAttrs flag and the stored imp data. And then the map in AuctionRequest should probably be called something like ImpExtInfoMap

mansinahar · 2021-07-28T00:24:27Z

exchange/exchange.go

@@ -135,6 +136,11 @@ func NewExchange(adapters map[openrtb_ext.BidderName]adaptedBidder, cache prebid
 	}
 }

+type StoredImpData struct {
+	IncludeVideoAttributes bool
+	Data                   []byte


And this should the probably be called StoredImpData or just StoredImp

mansinahar · 2021-07-29T11:37:05Z

endpoints/openrtb2/auction.go

@@ -192,6 +192,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http
 		LegacyLabels:               labels,
 		Warnings:                   warnings,
 		GlobalPrivacyControlHeader: secGPC,
+		ImpToStoredReq:             impExtInfoMap,


@VeronikaSolovei9 Looks like you missed renaming this field here in AuctionRequest

Yes, renamed.

endpoints/openrtb2/auction.go

endpoints/openrtb2/auction_test.go

exchange/exchange.go

exchange/exchange_test.go

endpoints/openrtb2/auction.go

SyntaxNode · 2021-07-29T16:46:54Z

endpoints/openrtb2/auction_test.go

+	for i, requestData := range testStoredRequestsErrors {
+		_, _, errList := deps.processStoredRequests(context.Background(), json.RawMessage(requestData))
+		if len(errList) == 0 {
+			t.Error("processStoredRequests should return error")


Does t.Error stop the running test - or is that t.Fatal?

For some reason I cannot answer to your comment about resolvedImp.
At this moment we already have final imp in byte array, but don't have it in object yet.
resolvedImp, err := jsonpatch.MergePatch(storedImps[impIds[i]], imps[idIndices[i]])
We have raw data here that we want to return in response.

t.Error reports error in logs, but doesn't stop the execution

SyntaxNode · 2021-07-29T16:47:42Z

endpoints/openrtb2/auction_test.go

+
+var testStoreVideoAttr = []bool{
+	true, true, false, false,
+}


Style Nitpick: Consider making the slices definitions in-line.

mansinahar · 2021-08-03T13:23:55Z

exchange/exchange.go

@@ -935,7 +943,7 @@ func (e *exchange) makeBid(bids []*pbsOrtbBid, auc *auction, returnCreative bool
 			}
 		}

-		if bidExtJSON, err := makeBidExtJSON(bid.bid.Ext, bidExtPrebid); err != nil {
+		if bidExtJSON, err := makeBidExtJSON(bid.bid.Ext, bidExtPrebid, impExtInfoMap[bid.bid.ImpID]); err != nil {


Don't we need to first check if impExtInfoMap even contains an element with bid.bid.ImpID before we pass it to makeBidExtJSON? Because not all imps might have a stored Imp

mansinahar · 2021-08-03T13:26:52Z

exchange/exchange.go

-		return nil, err
+
+	if len(ext) != 0 {
+		// update existing bid.ext with our prebid section. if bid.ext.prebid already exists, it will be overwritten.


Please update this comment to incorporate for your changes about video attributes

This comment is still valid.
if bid.ext.prebid already exists, it will be overwritten - this is what it does.
EchoVideoAttrs will be inserted in bid.ext not bid.ext.prebid in further processing.
I'm open to modify this comment to anything better, please post your suggestions.

Sorry, I should have been more clearer. I agree with what you said above. What I meant by my comment was that maybe we should extend that comment to also add a note about the video attributes. Something like:

// update existing bid.ext with our prebid section and add stored video // attributes for imp if echoVideoAttrs set to true. // if bid.ext.prebid already exists, it will be overwritten.

Also, perhaps this comment makes more sense to be on the first line of this function

mansinahar · 2021-08-03T13:29:03Z

exchange/exchange.go

+
+	if impExtInfo.EchoVideoAttrs {
+		videoData, _, _, err := jsonparser.Get(impExtInfo.StoredImp, "video")
+		if err != nil {


Should we ignore the KeyPathNotFoundError here because a stored imp might not always have a video part and if it doesn't I assume we'd just wanna continue rather than return an error, no?

If request is valid, there shouldn't be KeyPathNotFoundError. But I agree this case needs to be handled. Unit test added as well. Good catch!

mansinahar · 2021-08-03T13:40:10Z

exchange/exchange.go

 )

 type ContextKey string

 const DebugContextKey = ContextKey("debugInfo")
+const StoredRequestAttributes = "storedrequestattributes"


Do you think openrtb_ext/bid.go would make a better place for this as we're using this value as a key in the bid ext?

I was also thinking about it too. And the thing is that PrebidExtKey constant is located in openrtb_ext/device.go. There are no bid.ext constants in bid.go. Not sure where this one should go.

Yeah, I was wondering the same about PrebidExtKey but PrebidExtKey is a constant key that is valid for different extension objects such as bid and imp. So we want to have that in a common place. That common place definitely shouldn't be device.go.. maybe an openrtb_ext.go file that can hold common stuff like this but that's for another PR some other time :D. For now, I think StoredRequestAttributes would make more sense in openrtb_ext/bid.go since that is specific to bid ext.

mansinahar · 2021-08-03T13:47:13Z

exchange/exchange_test.go

+			impExtInfo:         ImpExtInfo{true, []byte(`{"video":{!}}`)},
+			expectedBidExt:     ``,
+			expectedErrMessage: "invalid character",
+		},


Please consider adding a test case where the impExtInfo doesn't have a video part in it. Also, this isn't exactly your change but if it's not a lot of effort can I please request to add a test where ext has a prebid part and that gets overwritten by extBidPrebid? Thanks!

Yes, added.

mansinahar · 2021-08-03T13:51:46Z

exchange/exchange_test.go

@@ -1400,7 +1400,7 @@ func TestBidReturnsCreative(t *testing.T) {

 	//Run tests
 	for _, test := range testCases {
-		resultingBids, resultingErrs := e.makeBid(sampleBids, sampleAuction, test.inReturnCreative)


Might wanna modify one of these valid buildBidResponse test cases to include impExtInfo and make sure that it is propagated correctly downstream and we are getting the expected values back in bid ext.

Added new unit test for this: TestMakeBidsVideoAttributes

I am not sure if we need TestMakeBidsVideoAttributes because all those test cases are already being tested in the TestMakeBidExtJSON test that you added. In makeBids we only pass impExtInfo downstream, we don't really have any logic addition/deletion/modification there. In such cases I'd just add one new test case for that upstream method (in this case makeBids) to make sure the value is being passed downstream as expected. Exhaustive test cases about the downstream method (in this case makeBidExtJSON) where the actual logic is changed makes sense only for the test for that method. This is why I recommended that we just add a new test case for buildBidResponse or modify and existing test case for buildBidResponse to just make sure that the value is properly passed all the way downstream to makeBidExtJSON.

I hope this makes sense!

mansinahar · 2021-08-03T14:17:49Z

endpoints/openrtb2/auction_test.go

+
+	for i, requestData := range testStoredRequestsErrors {
+		_, _, errList := deps.processStoredRequests(context.Background(), json.RawMessage(requestData))
+		if len(errList) == 0 {


Nitpick: I'd recommend using assert.Len here rather. I know there are parts of our code where we don't use the assert library for test assertions but if we can do that going forward to keep consistency then that'd be great. The assert library makes the test assertion code a little leaner and much easier to read

Modified it to assert.Empty, but personally len(errList) == 0 is easier to read for me.

mansinahar · 2021-08-03T14:21:28Z

endpoints/openrtb2/auction_test.go

 		}
+		err := errList[0]
+		assert.Equal(t, testStoredRequestsErrorsResults[i], err.Error(), "Incorrect error")


You can rather use assert.Contains here so that you don't need to hardcode always looking at the first error in the slice. If the code changes in the future which may result in that error to move further down the slice then this would break even though the slice would still contain the expected error

Yes, my bad I usually use assert.Contains.

endpoints/openrtb2/auction.go

exchange/exchange.go

mansinahar · 2021-08-04T13:14:22Z

exchange/exchange_test.go

@@ -1400,7 +1400,7 @@ func TestBidReturnsCreative(t *testing.T) {

 	//Run tests
 	for _, test := range testCases {
-		resultingBids, resultingErrs := e.makeBid(sampleBids, sampleAuction, test.inReturnCreative)


I am not sure if we need TestMakeBidsVideoAttributes because all those test cases are already being tested in the TestMakeBidExtJSON test that you added. In makeBids we only pass impExtInfo downstream, we don't really have any logic addition/deletion/modification there. In such cases I'd just add one new test case for that upstream method (in this case makeBids) to make sure the value is being passed downstream as expected. Exhaustive test cases about the downstream method (in this case makeBidExtJSON) where the actual logic is changed makes sense only for the test for that method. This is why I recommended that we just add a new test case for buildBidResponse or modify and existing test case for buildBidResponse to just make sure that the value is properly passed all the way downstream to makeBidExtJSON.

I hope this makes sense!

mansinahar · 2021-08-04T13:20:01Z

exchange/exchange.go

-		return nil, err
+
+	if len(ext) != 0 {
+		// update existing bid.ext with our prebid section. if bid.ext.prebid already exists, it will be overwritten.


Sorry, I should have been more clearer. I agree with what you said above. What I meant by my comment was that maybe we should extend that comment to also add a note about the video attributes. Something like:

// update existing bid.ext with our prebid section and add stored video // attributes for imp if echoVideoAttrs set to true. // if bid.ext.prebid already exists, it will be overwritten.

Also, perhaps this comment makes more sense to be on the first line of this function

mansinahar · 2021-08-04T13:25:47Z

exchange/exchange.go

 )

 type ContextKey string

 const DebugContextKey = ContextKey("debugInfo")
+const StoredRequestAttributes = "storedrequestattributes"


Yeah, I was wondering the same about PrebidExtKey but PrebidExtKey is a constant key that is valid for different extension objects such as bid and imp. So we want to have that in a common place. That common place definitely shouldn't be device.go.. maybe an openrtb_ext.go file that can hold common stuff like this but that's for another PR some other time :D. For now, I think StoredRequestAttributes would make more sense in openrtb_ext/bid.go since that is specific to bid ext.

endpoints/openrtb2/auction_test.go

VeronikaSolovei9 · 2021-08-04T20:36:48Z

Comments addressed. please take another look

SyntaxNode · 2021-08-04T21:01:34Z

endpoints/openrtb2/auction.go

@@ -135,7 +135,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http
 		deps.analytics.LogAuctionObject(&ao)
 	}()

-	req, errL := deps.parseRequest(r)
+	req, impExtInfoMap, errL := deps.parseRequest(r)


Nitpick: Looking at the code a bit more with the new flow, how do you feel about naming this impExtInfoByID to provide context of the map key? I don't feel strongly about this if you feel differently.

We agreed with this name with Mansi earlier. I personally like impExtInfoMap more, but I'm open to rename it. @mansinahar what do you think?

Both of those names sound good to me. I don't feel strongly about either. Will leave it up to you Veronika.

endpoints/openrtb2/auction.go

endpoints/openrtb2/auction_test.go

exchange/exchange_test.go

exchange/exchange.go

endpoints/openrtb2/auction.go

mansinahar

LGTM! Thanks for being patient with this one and addressing all the feedback @VeronikaSolovei9. Appreciate it :)

SyntaxNode reviewed Jun 15, 2021

View reviewed changes

endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

SyntaxNode self-assigned this Jun 16, 2021

SyntaxNode requested a review from mansinahar June 16, 2021 17:20

SyntaxNode assigned mansinahar Jun 16, 2021

SyntaxNode reviewed Jun 16, 2021

View reviewed changes

endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

mansinahar reviewed Jun 21, 2021

View reviewed changes

exchange/exchange.go Outdated Show resolved Hide resolved

endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved

SyntaxNode reviewed Jun 21, 2021

View reviewed changes

exchange/exchange.go Outdated Show resolved Hide resolved

SyntaxNode reviewed Jun 29, 2021

View reviewed changes

mansinahar reviewed Jul 6, 2021

View reviewed changes

Veronika Solovei added 11 commits July 21, 2021 14:55

Mirror stored imp data to bid response

ea90f1d

Race condition fix

c35805c

Code refactoring

ddaebca

Code refactoring

ca21a86

Code refactoring

6b3ff21

Code refactoring

1c40e94

Code refactoring

279b5cb

Code refactoring

6bbfab3

Code refactoring

0076fca

Added comment

c359a69

Code refactoring

e7e5324

VeronikaSolovei9 force-pushed the stored_imp branch from fe7c11a to e7e5324 Compare July 21, 2021 22:00

Code refactoring: moved stored request attributes to separate structure

90d3633

mansinahar reviewed Jul 28, 2021

View reviewed changes

Code refactoring: structure and variables rename

1aecfc6

mansinahar reviewed Jul 29, 2021

View reviewed changes

SyntaxNode reviewed Jul 29, 2021

View reviewed changes

Veronika Solovei added 3 commits July 29, 2021 22:08

Minor refactoring and unit test fix

957a451

Minor refactoring and unit test fix

a860f6a

Code clean up

25db034

Code refactoring

ceb1f43

mansinahar reviewed Aug 3, 2021

View reviewed changes

Veronika Solovei added 3 commits August 3, 2021 20:17

Code and unit tests refactoring

7b2f5f2

Code and unit tests refactoring

5a897b2

Unit tests fix

b912079

mansinahar reviewed Aug 4, 2021

View reviewed changes

Code clean up

3a3118a

SyntaxNode reviewed Aug 4, 2021

View reviewed changes

Code clean up

852ea18

mansinahar approved these changes Aug 5, 2021

View reviewed changes

SyntaxNode approved these changes Aug 5, 2021

View reviewed changes

SyntaxNode merged commit 6c3e7f0 into master Aug 5, 2021

VeronikaSolovei9 mentioned this pull request Aug 31, 2021

processStoredRequests refactoring #1961

Merged

jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021

Mirror stored imp data to bid response (prebid#1889)

dbcec80

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

Mirror stored imp data to bid response (prebid#1889)

4a98053

SyntaxNode deleted the stored_imp branch March 13, 2023 13:54

Mirror stored imp data to bid response #1889

Mirror stored imp data to bid response #1889

Conversation

VeronikaSolovei9 commented Jun 15, 2021 • edited by SyntaxNode

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 Jul 23, 2021

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 Aug 3, 2021 • edited

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 Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jun 15, 2021 •

edited by SyntaxNode

VeronikaSolovei9 Aug 3, 2021 •

edited