Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adserver targeting #2593

Merged
merged 19 commits into from
Apr 3, 2023
139 changes: 139 additions & 0 deletions adservertargeting/adservertargeting.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package adservertargeting

import (
"encoding/json"
"github.com/prebid/openrtb/v17/openrtb2"
"github.com/prebid/prebid-server/openrtb_ext"
"strings"
)

type DataSource string

const (
BidRequest DataSource = "bidrequest"
Static DataSource = "static"
BidResponse DataSource = "bidresponse"
)

const (
bidderMacro = "{{BIDDER}}"
pathDelimiter = "."
)

// RequestTargetingData struct to hold pre-processed ad server targeting keys and values
type RequestTargetingData struct {
SingleVal json.RawMessage
ImpData map[string][]byte
}

type ResponseTargetingData struct {
Key string
HasMacro bool
Path string
}

type adServerTargetingData struct {
RequestTargetingData map[string]RequestTargetingData
ResponseTargetingData []ResponseTargetingData
}

func ProcessAdServerTargeting(
reqWrapper *openrtb_ext.RequestWrapper,
resolvedRequest json.RawMessage,
response *openrtb2.BidResponse,
queryParams map[string]string,
bidResponseExt *openrtb_ext.ExtBidResponse,
truncateTargetAttribute *int) *openrtb2.BidResponse {

adServerTargeting, warnings := ExtractAdServerTargeting(reqWrapper, resolvedRequest, queryParams)
response, warnings = ResolveAdServerTargeting(adServerTargeting, response, warnings, truncateTargetAttribute)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!


if len(warnings) > 0 {
bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral] = append(bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral], warnings...)
}
return response
}

func ExtractAdServerTargeting(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming. This function iterates over each ad server targeting value and resolves it. Extract doesn't convey that behavior IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function collects targeting attributes for later processing. In main flow it is executed before final resolving:
func ProcessAdServerTargeting:

adServerTargeting, warnings := ExtractAdServerTargeting(reqWrapper, resolvedRequest, queryParams)
response, warnings = ResolveAdServerTargeting(adServerTargeting, response, warnings, truncateTargetAttribute)

Is CollectAdServerTargeting a good name for it?

Copy link
Contributor

@SyntaxNode SyntaxNode Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Naming is hard. How about collectTargetingValues or getTargetingValues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to collectTargetingValues

reqWrapper *openrtb_ext.RequestWrapper, resolvedRequest json.RawMessage,
ampData map[string]string) (*adServerTargetingData, []openrtb_ext.ExtBidderMessage) {
//this func should receive a finalized request wrapper

warnings := make([]openrtb_ext.ExtBidderMessage, 0)
adServerTargetingData := &adServerTargetingData{}
reqExt, err := reqWrapper.GetRequestExt()
if err != nil {
warnings = append(warnings, createWarning(err.Error()))
return nil, warnings
}
reqExtPrebid := reqExt.GetPrebid()
if reqExtPrebid == nil {
return nil, warnings
}
adServerTargeting := reqExtPrebid.AdServerTargeting

if len(adServerTargeting) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check necessary? If there's an error, it's handled by the above code. If adServerTargeting is empty, the for loop with gracefully exit. Is it important for a nil value to be returned instead of an empty adServerTargetingData struct? If that doesn't matter, you won't need to return a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Yes, this check is not needed. And nil value for adServerTargeting doesn't matter. I removed the check and left AdServerTargeting []AdServerTargeting as array, not as a pointer.

return nil, warnings
}

requestTargetingData := make(map[string]RequestTargetingData, 0)
responseTargetingData := make([]ResponseTargetingData, 0)

dataHolder := reqImpCache{resolverReq: resolvedRequest}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a different name than dataHolder. What data is it holding? Perhaps this should be valueCache or some such?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to impsCache and renamed similar variable for bids to bidCache


for _, targetingObj := range adServerTargeting {
source := strings.ToLower(targetingObj.Source)
switch DataSource(source) {
case BidRequest:
//causes PBS to treat 'value' as a path to pull from the request object
value, err := getValueFromBidRequest(&dataHolder, targetingObj.Value, ampData)
if err != nil {
warnings = append(warnings, createWarning(err.Error()))
continue
}
requestTargetingData[targetingObj.Key] = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring to remove the need for the continue keyword. Perhaps something like:

value, err := getValueFromBidRequest(&dataHolder, targetingObj.Value, queryParams)
if err != nil {
	warnings = append(warnings, createWarning(err.Error()))
} else {
	requestTargetingData[targetingObj.Key] = value
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

case Static:
// causes PBS to just use the 'value' provided
staticValue := RequestTargetingData{SingleVal: json.RawMessage(targetingObj.Value)}
requestTargetingData[targetingObj.Key] = staticValue
case BidResponse:
//causes PBS to treat 'value' as a path to pull from the bidder's response object, specifically seatbid[j].bid[k]
bidResponseTargeting := ResponseTargetingData{}
bidResponseTargeting.Key = targetingObj.Key
bidResponseTargeting.Path = targetingObj.Value
bidResponseTargeting.HasMacro = strings.Contains(strings.ToUpper(targetingObj.Key), bidderMacro)
responseTargetingData = append(responseTargetingData, bidResponseTargeting)
}

}

adServerTargetingData.RequestTargetingData = requestTargetingData
adServerTargetingData.ResponseTargetingData = responseTargetingData
return adServerTargetingData, warnings
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left nitpick comments, but overall I feel this function is well coded and easy to understand.


func ResolveAdServerTargeting(
adServerTargetingData *adServerTargetingData,
response *openrtb2.BidResponse,
warnings []openrtb_ext.ExtBidderMessage,
truncateTargetAttribute *int) (*openrtb2.BidResponse, []openrtb_ext.ExtBidderMessage) {
if adServerTargetingData == nil {
return response, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// ad server targeting data will go to seatbid[].bid[].ext.prebid.targeting
//TODO: truncate keys

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: can this blank line be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bidsHolder := bidsCache{bids: make(map[string]map[string][]byte)}
bidderRespHolder := respCache{}
for _, seat := range response.SeatBid {
bidderName := seat.Seat
for i, bid := range seat.Bid {
targetingData := make(map[string]string, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing with an object initializer or omit the initial size of 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 0 size initialization.

processRequestTargetingData(adServerTargetingData, targetingData, bid.ImpID)
processResponseTargetingData(adServerTargetingData, targetingData, bidderName, bid, bidsHolder, bidderRespHolder, response, seat.Ext, warnings)
seat.Bid[i].Ext = buildBidExt(targetingData, bid, warnings, truncateTargetAttribute)
}
}
return response, warnings
}
Loading