Skip to content

Simplifying exchange module: bidResponseExt gets built anyway #1518

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 15, 2020
Merged

Simplifying exchange module: bidResponseExt gets built anyway #1518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23a3e6e
first draft
Oct 1, 2020
8f07569
Scott's feedback
Oct 6, 2020
d0e8c83
stepping out real quick
Oct 8, 2020
1904b7b
add cache errors to bidResponseExt before marshalling
Oct 15, 2020
bbc0eb4
Merge branch 'master' into holdAuctionRefactor
Oct 15, 2020
07626db
Removed vim's swp file
Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 68 additions & 64 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ func NewExchange(client *http.Client, cache prebid_cache_client.Client, cfg *con
}

func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidRequest, usersyncs IdFetcher, labels pbsmetrics.Labels, account *config.Account, categoriesFetcher *stored_requests.CategoryFetcher, debugLog *DebugLog) (*openrtb.BidResponse, error) {
var auc *auction = nil
var bidResponseExt *openrtb_ext.ExtBidResponse = nil
var cacheErrs []error = nil
var err error

requestExt, err := extractBidRequestExt(bidRequest)
if err != nil {
Expand All @@ -114,34 +118,11 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque

bidAdjustmentFactors := getExtBidAdjustmentFactors(requestExt)

for _, impInRequest := range bidRequest.Imp {
var impLabels pbsmetrics.ImpLabels = pbsmetrics.ImpLabels{
BannerImps: impInRequest.Banner != nil,
VideoImps: impInRequest.Video != nil,
AudioImps: impInRequest.Audio != nil,
NativeImps: impInRequest.Native != nil,
}
e.me.RecordImps(impLabels)
}
e.recordImpMetrics(bidRequest)

// Make our best guess if GDPR applies
usersyncIfAmbiguous := e.UsersyncIfAmbiguous
var geo *openrtb.Geo = nil
if bidRequest.User != nil && bidRequest.User.Geo != nil {
geo = bidRequest.User.Geo
} else if bidRequest.Device != nil && bidRequest.Device.Geo != nil {
geo = bidRequest.Device.Geo
}
if geo != nil {
// If we have a country set, and it is on the list, we assume GDPR applies if not set on the request.
// Otherwise we assume it does not apply as long as it appears "valid" (is 3 characters long).
if _, found := e.privacyConfig.GDPR.EEACountriesMap[strings.ToUpper(geo.Country)]; found {
usersyncIfAmbiguous = false
} else if len(geo.Country) == 3 {
// The country field is formatted properly as a three character country code
usersyncIfAmbiguous = true
}
}
usersyncIfAmbiguous := e.parseUsersyncIfAmbiguous(bidRequest)

// Slice of BidRequests, each a copy of the original cleaned to only contain bidder data for the named bidder
blabels := make(map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels)
cleanRequests, aliases, privacyLabels, errs := cleanOpenRTBRequests(ctx, bidRequest, requestExt, usersyncs, blabels, labels, e.gDPR, usersyncIfAmbiguous, e.privacyConfig)
Expand All @@ -161,14 +142,11 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque

adapterBids, adapterExtra, anyBidsReturned := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)

var auc *auction = nil
var bidResponseExt *openrtb_ext.ExtBidResponse = nil
if anyBidsReturned {

var bidCategory map[string]string
//If includebrandcategory is present in ext then CE feature is on.
if requestExt.Prebid.Targeting != nil && requestExt.Prebid.Targeting.IncludeBrandCategory != nil {
var err error
var rejections []string
bidCategory, adapterBids, rejections, err = applyCategoryMapping(ctx, requestExt, adapterBids, *categoriesFetcher, targData)
if err != nil {
Expand All @@ -189,52 +167,79 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
errs = append(errs, dealErrs...)
}

if debugLog != nil && debugLog.Enabled {
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)
if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil {
debugLog.Data.Response = string(bidRespExtBytes)
} else {
debugLog.Data.Response = "Unable to marshal response ext for debugging"
errs = append(errs, errors.New(debugLog.Data.Response))
}
}

cacheErrs := auc.doCache(ctx, e.cache, targData, bidRequest, 60, &account.CacheTTL, bidCategory, debugLog)
if len(cacheErrs) > 0 {
errs = append(errs, cacheErrs...)
}
targData.setTargeting(auc, bidRequest.App != nil, bidCategory)

// Ensure caching errors are added if the bid response ext has already been created
if bidResponseExt != nil && len(cacheErrs) > 0 {
bidderCacheErrs := errsToBidderErrors(cacheErrs)
bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...)
}
}

}

if !anyBidsReturned {
if debugLog != nil && debugLog.Enabled {
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)
if debugLog != nil && debugLog.Enabled {
if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil {
debugLog.Data.Response = string(bidRespExtBytes)
} else {
debugLog.Data.Response = "Unable to marshal response ext for debugging"
errs = append(errs, err)
}
if !anyBidsReturned {
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'm still not sure why debugLog.CacheKey = rawUUID.String() only happens when !anyBidsReturned but I refactored to keep the exact same logic (please let me know if I you think I missed something).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing there's an assumption that the CacheKey will set elsewhere if there are bids. There seems to be a lot of code checking if it's length is 0. I'd be afraid to change that assumption in this PR. If you're up for it, a separate PR would be appropriate to fix that concept.

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 believe a non-nil debugLog object can only come from the video_auction.go endpoint and also see that when an error occurs in the video endpoint, a rawUUID, err := uuid.NewV4(); also gets generated just before exiting execution. My guess is that only when an error happens, or no bids are returned, the UUID to a debug log is returned.

In either case, it'd be better that @camrice or someone from his team takes a look to this refactoring PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The cache key creation can be removed here so only the response so far is added to the debugLog. Before, I believe I didn't have the cache key created in PutDebugLogError or handleError, so this case would be missed.

if rawUUID, err := uuid.NewV4(); err == nil {
debugLog.CacheKey = rawUUID.String()

bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)
if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil {
debugLog.Data.Response = string(bidRespExtBytes)
} else {
debugLog.Data.Response = "Unable to marshal response ext for debugging"
}
} else {
errs = append(errs, err)
}
}
}

// Ensure caching errors are added if the bid response ext has already been created
if len(cacheErrs) > 0 {
bidderCacheErrs := errsToBidderErrors(cacheErrs)
bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...)
}

// Build the response
return e.buildBidResponse(ctx, liveAdapters, adapterBids, bidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, errs)
}

func (e *exchange) parseUsersyncIfAmbiguous(bidRequest *openrtb.BidRequest) bool {
usersyncIfAmbiguous := e.UsersyncIfAmbiguous
var geo *openrtb.Geo = nil

if bidRequest.User != nil && bidRequest.User.Geo != nil {
geo = bidRequest.User.Geo
} else if bidRequest.Device != nil && bidRequest.Device.Geo != nil {
geo = bidRequest.Device.Geo
}
if geo != nil {
// If we have a country set, and it is on the list, we assume GDPR applies if not set on the request.
// Otherwise we assume it does not apply as long as it appears "valid" (is 3 characters long).
if _, found := e.privacyConfig.GDPR.EEACountriesMap[strings.ToUpper(geo.Country)]; found {
usersyncIfAmbiguous = false
} else if len(geo.Country) == 3 {
// The country field is formatted properly as a three character country code
usersyncIfAmbiguous = true
}
}

return usersyncIfAmbiguous
}

func (e *exchange) recordImpMetrics(bidRequest *openrtb.BidRequest) {

for _, impInRequest := range bidRequest.Imp {
var impLabels pbsmetrics.ImpLabels = pbsmetrics.ImpLabels{
BannerImps: impInRequest.Banner != nil,
VideoImps: impInRequest.Video != nil,
AudioImps: impInRequest.Audio != nil,
NativeImps: impInRequest.Native != nil,
}
e.me.RecordImps(impLabels)
}

}

type DealTierInfo struct {
Prefix string `json:"prefix"`
MinDealTier int `json:"minDealTier"`
Expand Down Expand Up @@ -479,6 +484,7 @@ func errsToBidderErrors(errs []error) []openrtb_ext.ExtBidderError {
// This piece takes all the bids supplied by the adapters and crafts an openRTB response to send back to the requester
func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb.BidRequest, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, returnCreative bool, errList []error) (*openrtb.BidResponse, error) {
bidResponse := new(openrtb.BidResponse)
var err error

bidResponse.ID = bidRequest.ID
if len(liveAdapters) == 0 {
Expand All @@ -500,21 +506,19 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_

bidResponse.SeatBid = seatBids

if bidResponseExt == nil {
contextDebugValue := ctx.Value(DebugContextKey)
var debugInfo bool
if contextDebugValue != nil {
debugInfo = contextDebugValue.(bool)
}
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errList)
}
bidResponse.Ext, err = encodeBidResponseExt(bidResponseExt)

return bidResponse, err
}

func encodeBidResponseExt(bidResponseExt *openrtb_ext.ExtBidResponse) ([]byte, error) {
buffer := &bytes.Buffer{}
enc := json.NewEncoder(buffer)

enc.SetEscapeHTML(false)
err := enc.Encode(bidResponseExt)
bidResponse.Ext = buffer.Bytes()

return bidResponse, err
return buffer.Bytes(), err
}

func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtRequest, seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, categoriesFetcher stored_requests.CategoryFetcher, targData *targetData) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []string, error) {
Expand Down
6 changes: 5 additions & 1 deletion exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,9 +955,13 @@ func TestBidResponseCurrency(t *testing.T) {
},
}

bidResponseExt := &openrtb_ext.ExtBidResponse{
ResponseTimeMillis: map[openrtb_ext.BidderName]int{openrtb_ext.BidderName("appnexus"): 5},
RequestTimeoutMillis: 500,
}
// Run tests
for i := range testCases {
actualBidResp, err := e.buildBidResponse(context.Background(), liveAdapters, testCases[i].adapterBids, bidRequest, adapterExtra, nil, nil, true, errList)
actualBidResp, err := e.buildBidResponse(context.Background(), liveAdapters, testCases[i].adapterBids, bidRequest, adapterExtra, nil, bidResponseExt, true, errList)
assert.NoError(t, err, fmt.Sprintf("[TEST_FAILED] e.buildBidResponse resturns error in test: %s Error message: %s \n", testCases[i].description, err))
assert.Equalf(t, testCases[i].expectedBidResponse, actualBidResp, fmt.Sprintf("[TEST_FAILED] Objects must be equal for test: %s \n Expected: >>%s<< \n Actual: >>%s<< ", testCases[i].description, testCases[i].expectedBidResponse.Ext, actualBidResp.Ext))
}
Expand Down