Adserver targeting #2593

VeronikaSolovei9 · 2023-02-16T09:46:16Z

Implementation of #1167

adservertargeting/adservertargeting.go

SyntaxNode · 2023-03-03T17:27:17Z

adservertargeting/respdataprocessor.go

+	// path points to value in response, not seat or ext
+	// bidder response has limited number of properties
+	// instead of unmarshal the whole response with seats and bids
+	// try to find data by field name


Please move comments like this up a level on getValueFromResp directly.

Agree, moved

Not really what I was thinking, but it could work too. I meant to suggest something like this:

// getValueFromResp optimizes retrieval of paths for a bid response (not a seat or ext) func getValueFromResp(

Oh ok, modified

adservertargeting/adservertargeting.go

adservertargeting/respcache.go

SyntaxNode · 2023-03-03T17:57:51Z

adservertargeting/respcache.go

+func (brh *respCache) GetRespData(bidderResp *openrtb2.BidResponse, field string) (string, error) {
+	if len(brh.bidRespData) == 0 {
+		// this code should be modified if there are changes in Op[enRtb format
+		// it's up to date with OpenRTB 2.5 and 2.6


This comment will not be adhered to. You should add a test to fail when the assumptions made by this function change. I can reach out separately to help construct a test like that.

I left a comment for now. Please reach me out to discuss how to create this test, it would be good to have it.

adservertargeting/respdataprocessor.go

util/httputil/httputil.go

SyntaxNode

Excellent update! This is a lot easier to follow and appears to match the spec. I would like to see end to end tests to ensure this is hooked into the main auction flow correctly and to provide examples on the feature's usage.

The only feature from the spec I didn't see represented is:

If the value points to a JSON path that is an object or array, there should be a warning in debug mode

Is that already accounted for? .. and if so, where?

SyntaxNode · 2023-03-10T19:55:58Z

exchange/exchange.go

@@ -425,6 +427,11 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *

 	// Build the response
 	bidResponse, err := e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequestWrapper.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, r.ImpExtInfoMap, r.PubID, errs)
+
+	bidResponse = adservertargeting.ProcessAdServerTargeting(r.BidRequestWrapper, r.ResolvedBidRequest, bidResponse, r.QueryParams, bidResponseExt, r.Account.TruncateTargetAttribute)


The "ad server targeting" information from the package name is repeated in the function call. To avoid, consider renaming ProcessAdServerTargeting to Apply, which then produces the simplified code:

bidResponse = adservertargeting.Appy(r.BidRequestWrapper, r.ResolvedBidRequest, bidResponse, r.QueryParams, bidResponseExt, r.Account.TruncateTargetAttribute)

Yes, renamed!

SyntaxNode · 2023-03-10T19:58:16Z

exchange/exchange_test.go

@@ -1711,8 +1711,6 @@ func TestBidResponseCurrency(t *testing.T) {
 				ID:      "some-request-id",
 				SeatBid: sampleSeatBid,
 				Cur:     "USD",
-				Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}


I see you moved the call to encodeBidResponseExt. I assume these tests are only in scope of the old location. Is there test coverage of encodeBidResponseExt in its new location?

There were no tests for encodeBidResponseExt. It was tested together with buildBidResponse function.
Func encodeBidResponseExt is now 100% covered by many other exchange tests including json tests. Do you want me to add specific test for encodeBidResponseExt?

SyntaxNode · 2023-03-10T19:59:03Z

openrtb_ext/request.go

@@ -71,6 +71,15 @@ type ExtRequestPrebid struct {
 	// - basic: excludes debugmessages and analytic_tags from output
 	// any other value or an empty string disables trace output at all.
 	Trace string `json:"trace,omitempty"`
+
+	//
+	AdServerTargeting []AdServerTargeting `json:"adservertargeting,omitempty"`


No comment is necessary here IMHO. Feel free to remove the stub.

I would suggest a slight renaming, to avoid thing = list of things. Either AdServerTargeting []AdServerTarget, AdServerTargeting []AdServerTargetRule, or AdserverTargets []AdServerTargeting

renamed to AdServerTargeting []AdServerTarget

SyntaxNode · 2023-03-10T20:00:28Z

adservertargeting/adservertargeting.go

+	truncateTargetAttribute *int) *openrtb2.BidResponse {
+
+	adServerTargeting, warnings := CollectAdServerTargeting(reqWrapper, resolvedRequest, queryParams)
+	response, warnings = ResolveAdServerTargeting(adServerTargeting, response, warnings, truncateTargetAttribute)


The functions CollectAdServerTargeting and ResolveAdServerTargeting are no longer used outside of this package. You're welcomed to simplify their names and and make them package scoped. Perhaps collect and resolve?

SyntaxNode · 2023-03-10T20:00:59Z

adservertargeting/adservertargeting.go

+	return response
+}
+
+func CollectAdServerTargeting(


Please add a comment to help the reader understand what this function is doing. Even if you change it to package scope.

SyntaxNode · 2023-03-10T20:22:50Z

adservertargeting/reqdataprocessor.go

@@ -0,0 +1,104 @@
+package adservertargeting


The term "processor" is not descriptive of the functionality IMHO. How about something like "requestlookup"? .. or just a description which better translates to "looks up and returns a value".

SyntaxNode · 2023-03-10T20:23:19Z

adservertargeting/reqdataprocessor.go

+		return nil, err
+	}
+	return reqExtPrebid.AdServerTargeting, nil
+}


Consider adding line breaks between the error checking:

reqExt, err := reqWrapper.GetRequestExt() if err != nil { return nil, err } reqExtPrebid := reqExt.GetPrebid() if reqExtPrebid == nil { return nil, err } return reqExtPrebid.AdServerTargeting, nil

SyntaxNode · 2023-03-10T20:24:18Z

adservertargeting/respdataprocessor.go

+const MaxKeyLength = 20
+
+func processRequestTargetingData(adServerTargetingData *adServerTargetingData, targetingData map[string]string, bidImpId string) {
+	if len(adServerTargetingData.RequestTargetingData) == 0 {


The length check isn't necessary. Keep in mind that Go's range loop gracefully handles nil or empty lists such that no optimization is necessary.

SyntaxNode · 2023-03-10T20:24:55Z

adservertargeting/respdataprocessor.go

+	warnings []openrtb_ext.ExtBidderMessage) {
+	if len(adServerTargetingData.ResponseTargetingData) == 0 {
+		return
+	}


Same deal here. No need for the length check.

Right, removed.

SyntaxNode · 2023-03-10T20:33:10Z

adservertargeting/respdataprocessor.go

+
+	}
+	targetingData[key] = value
+}


IMHO it might be easier to read the method by moving the parameters to a single line. I've also noticed there is no return values which increases the number of arguments. How do you feel about:

func getValueFromSeatBidBid(path string, targetingData map[string]string, bidsHolder bidsCache, bidderName string, bid openrtb2.Bid) error { bidBytes, err := bidsHolder.GetBid(bidderName, bid.ID, bid) if err != nil { return createWarning(err.Error()) } path = strings.TrimPrefix(path, "seatbid.bid.") value, err := splitAndGet(path, bidBytes, pathDelimiter) if err != nil { return createWarning(err.Error()) } return value }

where the call site now looks like:

if v, err := getValueFromSeatBidBid(path, targetingData, bidsHolder, bidderName, bid); err == nil { targetingData[key] = v } else { warnings = append(warnings, err) }

Such that the responsibility of setting the response and collecting errors is moved up a level. Same comment applies to the other getValue methods.

This is a great point! How I didn't see it while coding (I wanted to hide all those appends and set to map statements)... It's even better, error check and value set is only required once for all "getValue" functions at the end of processResponseTargetingData

# Conflicts: # errortypes/code.go # openrtb_ext/bid.go # openrtb_ext/request.go

VeronikaSolovei9 · 2023-03-14T06:24:27Z

Need to add end to end test.

…added end to end unit tests

AlexBVolcy · 2023-03-16T19:40:40Z

adservertargeting/adservertargeting.go

+	warnings []openrtb_ext.ExtBidderMessage,
+	truncateTargetAttribute *int) (*openrtb2.BidResponse, []openrtb_ext.ExtBidderMessage) {
+	if adServerTargetingData == nil {
+		return response, nil


Is there a way to cover this line with a test?

Good point. I realized this will never be the case and removed the check.

AlexBVolcy · 2023-03-16T19:41:29Z

adservertargeting/adservertargeting.go

+	}
+
+	// ad server targeting data will go to seatbid[].bid[].ext.prebid.targeting
+


Nitpick: can this blank line be removed?

I removed the entire comment. I don't think it's needed.

AlexBVolcy · 2023-03-16T19:43:45Z

adservertargeting/reqcache.go

+	impsData    []json.RawMessage
+}
+
+func (dh *reqImpCache) GetReqJson() []byte {


Could the variable dh get a more descriptive name? Is it supposed to represent dataHolder?

AlexBVolcy · 2023-03-16T19:44:54Z

adservertargeting/reqcache.go

+		impToBid := make(map[string][]byte, 0)
+		bdh.bids[bidderName] = impToBid
+	}
+	_, biddExists := bdh.bids[bidderName][bidId]


Nitpick: the variable biddExists is mispelled

AlexBVolcy · 2023-03-16T19:47:57Z

adservertargeting/requestlookup.go

+		return res, err
+	}
+
+	return res, nil


Could this line be covered by tests?

I tried and it looks like no. This case is logically unreachable, this line exists to satisfy the compiler requirement to have a return statement.
All "suspicious" cases will be handled and returned with a proper error message in the code statement above: getDataFromRequest

AlexBVolcy · 2023-03-16T19:52:12Z

adservertargeting/respdataprocessor.go

+		switch pathSplit[0] {
+
+		case "seatbid":
+			switch pathSplit[1] {


Is there any check needed here to ensure pathSplit[1] exists? Does a split call above always give us at least 2 values?

Good point! Path split may have just 1 element, for instance "cur". It means the value should be taken from bid response itself, like "resp.cur". However I added targeting keys path validation to "collect" function to make sure it's not empty.

AlexBVolcy · 2023-03-16T19:54:42Z

adservertargeting/respdataprocessor.go

+	if truncateTargetAttribute != nil {
+		maxLength = *truncateTargetAttribute
+		if maxLength < 0 {
+			maxLength = MaxKeyLength


Could we cover this line with a test?

AlexBVolcy · 2023-03-16T19:56:09Z

adservertargeting/respdataprocessor.go

+}
+
+func getRespData(bidderResp *openrtb2.BidResponse, field string) (string, error) {
+	// this code should be modified if there are changes in Op[enRtb format


Nitpick: Mispelling Op[enRtb --> OpenRtb

AlexBVolcy · 2023-03-16T19:57:58Z

adservertargeting/respdataprocessor.go

+	case "id":
+		return bidderResp.ID, nil
+	case "bidid":
+		return bidderResp.BidID, nil
+	case "cur":
+		return bidderResp.Cur, nil
+	case "customdata":
+		return bidderResp.CustomData, nil
+	case "nbr":
+		return fmt.Sprint(bidderResp.NBR.Val()), nil
+
+	default:
+		return "", errors.Errorf("key not found for field in bid response: %s", field)


This block needs test coverage

Added. And found and fixed related bug.

VeronikaSolovei9 · 2023-03-17T04:05:44Z

Good comments, Alex! Please re-review

hhhjort · 2023-03-20T20:20:21Z

adservertargeting/requestlookup.go

+
+	reqExtPrebid := reqExt.GetPrebid()
+	if reqExtPrebid == nil {
+		return nil, err


What error are you returning here? Is it possible to get to this line when err != nil?

Right, no error, refactored

hhhjort · 2023-03-20T20:50:55Z

adservertargeting/respdataprocessor.go

+	warnings []openrtb_ext.ExtBidderMessage,
+	truncateTargetAttribute *int) json.RawMessage {
+
+	targetingDataTruncated := truncateTargetingKeys(targetingData, truncateTargetAttribute)


Wouldn't it be better to truncate the keys back when we create them, rather than doing this loop through all the keys again?

My idea here was to separate "get values" logic from everything else, because "get logic" became complicated.
Func truncateTargetingKeys will be needed to execute for "request" keys and for "response" keys.
Also I'll need to pass truncateTargetAttribute parameter downstream to where we set keys and values to final map, I think it will overload logic and number of parameters.

I think having this logic here makes the code easier to understand.
I'm open to discuss moving it to another place, if you feel otherwise.

hhhjort · 2023-03-20T21:10:20Z

endpoints/openrtb2/sample-requests/hooks/auction_bidder_reject.json

@@ -51,8 +51,7 @@
          {
            "id": "applogy-bid",
            "impid": "some-impression-id",
-            "price": 0,
-            "ext": {"prebid":{"type":"banner"}}


Why did this ext need to be removed?

It appears bid.ext was not validated in unit tests before.
Actual bid.ext turns out to be

{"origbidcpm":0,"origbidcur":"USD","prebid":{"meta":{"adaptercode":"applogy"},"type":"banner"}}

not

"ext": {"prebid":{"type":"banner"}}

After I added bid.ext comparison logic it stoped working.
File auction_test.go -> func assertBidResponseEqual:

if len(expectedBid.Ext) > 0 { assert.JSONEq(t, string(expectedBid.Ext), string(actualBidMap[bidID].Ext), "Incorrect bid extension") }

I removed bid.ext from expected result, because it was not validated before and it was not valid anyways.
I can add expected bid.ext back and modify them accordingly. So far it's filled with default values only.

# Conflicts: # exchange/exchange_test.go

AlexBVolcy

Looks good! Left one small comment.

AlexBVolcy · 2023-03-21T21:20:31Z

adservertargeting/requestlookup.go

+	}
+
+	// get data by key from request
+	requestValue, err := getDataFromRequest(path, dataHolder)


Nitpick: the function names getValueFromBidRequest vs. getDataFromRequest are pretty similar and I'm wondering if there's a better name for one of them that'll help distinguish the two functions? If you feel fine about them then I'm happy to keep them as they are, but thought it was worth a mention.

Yes, understand. in this function we try to get a key that was not found in query param or in imps. this is a default case. Renamed to getDataFromRequestJson

# Conflicts: # exchange/exchange.go # openrtb_ext/request.go

AlexBVolcy · 2023-03-28T23:55:47Z

adservertargeting/requestcache_test.go

+		if test.expectedError {
+			assert.Error(t, err, "unexpected error returned")
+		} else {
+			assert.NoError(t, err, "expected error not returned")


Since what you're looking for is for no error to be returned, should the message be something like "unexpected error returned" I see you use that message for other NoError assertions in other parts of testing.

Yes, these should be swapped.

AlexBVolcy · 2023-03-29T00:05:34Z

adservertargeting/requestcache_test.go

+const (
+	reqValid = `{
+  "id": "req_id",
+  "imp": [
+    {
+      "id": "test_imp1",
+      "ext": {"appnexus": {"placementId": 250419771}},
+      "banner": {"format": [{"h": 250, "w": 300}]}
+    },
+    {
+      "id": "test_imp2",
+      "ext": {"appnexus": {"placementId": 250419771}},
+      "banner": {"format": [{"h": 250, "w": 300}]}
+    }
+  ],
+  "site": {"page": "test.com"}
+}`
+
+	reqInvalid = `{
+  "id": "req_id",
+  "imp": {
+	 "incorrect":true
+   },
+  "site": {"page": "test.com"}
+}`
+
+	reqNoImps = `{
+  "id": "req_id",
+  "site": {"page": "test.com"}
+}`
+)


Would it be too tedious to extract this consts to their own json files? I'm happy with whatever you think, but it might make tests a little cleaner.

I like this idea!

AlexBVolcy · 2023-03-29T00:11:15Z

adservertargeting/requestlookup_test.go

+			expectedError:     false,
+		},
+		{
+			description:       "valid request with correct ext.prebid, no ad server targeting",


I think this description needs to be updated? I think I see ad server targeting present in request ext.

Right, I modified the description.

AlexBVolcy · 2023-03-30T00:37:27Z

This looks good! I'm interested to see what @hhhjort comments before I approve.

hhhjort

LGTM

VeronikaSolovei9 force-pushed the adserver_targeting branch from b9cbc78 to 6c94e28 Compare March 2, 2023 21:14

AdServer Targeting implementation

ea03cb9

VeronikaSolovei9 force-pushed the adserver_targeting branch from 6c94e28 to ea03cb9 Compare March 2, 2023 21:21

VeronikaSolovei9 marked this pull request as ready for review March 2, 2023 21:21

VeronikaSolovei9 changed the title ~~[DRAFT] Adserver targeting~~ Adserver targeting Mar 2, 2023

SyntaxNode reviewed Mar 3, 2023

View reviewed changes

Code review fixes

3dc5388

SyntaxNode reviewed Mar 10, 2023

View reviewed changes

SyntaxNode self-assigned this Mar 13, 2023

vsolovei added 3 commits March 13, 2023 19:41

Merge branch 'master' into adserver_targeting

f232378

# Conflicts: # errortypes/code.go # openrtb_ext/bid.go # openrtb_ext/request.go

Code refactoring

8e07613

Added data type validation

fbe9803

Data type validation improvement

2a4bd75

bsardo assigned AlexBVolcy Mar 14, 2023

Fixed bug with empty r.ResolvedBidRequest if debug is disallowed and …

98b22ae

…added end to end unit tests

AlexBVolcy reviewed Mar 16, 2023

View reviewed changes

Code improvements and bugfix

959232d

bsardo assigned hhhjort and unassigned SyntaxNode Mar 20, 2023

hhhjort reviewed Mar 20, 2023

View reviewed changes

vsolovei added 2 commits March 20, 2023 18:49

Minor refactoring

161770a

Merge branch 'master' into adserver_targeting

d4c8652

# Conflicts: # exchange/exchange_test.go

AlexBVolcy reviewed Mar 21, 2023

View reviewed changes

vsolovei added 3 commits March 22, 2023 16:45

Minor refactoring

b0a898a

Unit tests part I

5af64d4

Unit tests part II

d02fd50

vsolovei added 4 commits March 23, 2023 23:34

Unit tests part III

4a2ae2d

Unit tests part IV

5f34967

Merge branch 'master' into adserver_targeting

20daaaf

# Conflicts: # exchange/exchange.go # openrtb_ext/request.go

Merge conflicts fixes

d4e501c

AlexBVolcy reviewed Mar 29, 2023

View reviewed changes

vsolovei added 2 commits March 28, 2023 23:50

Unit tests improvements

ccff0cf

Renamed constants

54c320c

hhhjort approved these changes Mar 30, 2023

View reviewed changes

AlexBVolcy approved these changes Mar 30, 2023

View reviewed changes

bsardo merged commit a81f4bd into master Apr 3, 2023

cross-repo-issue-creation bot mentioned this pull request Apr 3, 2023

Port PR from PBS-Go: Adserver targeting prebid/prebid-server-java#2268

Closed

VeronikaSolovei9 deleted the adserver_targeting branch May 24, 2023 22:17

blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023

Adserver targeting (prebid#2593)

60bf5de

		}

		// ad server targeting data will go to seatbid[].bid[].ext.prebid.targeting

Adserver targeting #2593

Adserver targeting #2593

Conversation

VeronikaSolovei9 commented Feb 16, 2023

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

SyntaxNode 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

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 14, 2023

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 Mar 17, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Mar 29, 2023 • 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

AlexBVolcy commented Mar 30, 2023

hhhjort left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 Mar 29, 2023 •

edited

Loading