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.

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

Jump to bottom

Simplifying exchange module: bidResponseExt gets built anyway #1518

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 2 commits
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
131 changes: 67 additions & 64 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ 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 err error
requestExt, err := extractBidRequestExt(bidRequest)
if err != nil {
return nil, err
Expand All @@ -114,34 +115,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)
}
recordImpMetrics(bidRequest, e.me)

// 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 +139,13 @@ 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
var auc *auction
var cacheErrs []error
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 +166,79 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
errs = append(errs, dealErrs...)
}

if debugLog != nil && debugLog.Enabled {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
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 in case auc.doCache was called and errors were returned
if len(cacheErrs) > 0 {
bidderCacheErrs := errsToBidderErrors(cacheErrs)
bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this block be between lines 178 and 179 where we call makeExtBidResponse and marshal the ext if the debugging is enabled? In that case a cache error would be in the marshaled ext.

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 think you have a point. The current version of the code places this block after we make and marshal the bidResponseExt, therefore, I respected that order. But it'd probably be a good idea to reconsider this in its own PR.

 97 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) {
 98
 99 *-- 92 lines: requestExt, err := extractBidRequestExt(bidRequest)---------------------------------------------------------------------------
191
192             if debugLog != nil && debugLog.Enabled {
193                 bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)
194                 if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil {
195                     debugLog.Data.Response = string(bidRespExtBytes)
196                 } else {
197                     debugLog.Data.Response = "Unable to marshal response ext for debugging"
198                     errs = append(errs, errors.New(debugLog.Data.Response))
199                 }
200             }
201
202             cacheErrs := auc.doCache(ctx, e.cache, targData, bidRequest, 60, &account.CacheTTL, bidCategory, debugLog)
203             if len(cacheErrs) > 0 {
204                 errs = append(errs, cacheErrs...)
205             }
206             targData.setTargeting(auc, bidRequest.App != nil, bidCategory)
207
208             // Ensure caching errors are added if the bid response ext has already been created
209             if bidResponseExt != nil && len(cacheErrs) > 0 {
210                 bidderCacheErrs := errsToBidderErrors(cacheErrs)
211                 bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...)
212             }
213         }
214
215     }
216
217 *-- 17 lines: if !anyBidsReturned {---------------------------------------------------------------------------------------------------------
234     // Build the response
235     return e.buildBidResponse(ctx, liveAdapters, adapterBids, bidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, errs)
236 }
exchange/exchange.go

What do you think?

Copy link
Collaborator

@bsardo bsardo Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A separate PR would be fine but I don't see the change I'm suggesting.

I figured it might be best for the following steps to happen in this order:

  1. Make the ext bid response:
    bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)
  2. Set any cache errors on the bid response:
    bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...)
  3. Marshal the bid response and then store it in the debug log data response:
    if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil { debugLog.Data.Response = string(bidRespExtBytes)

This way any cache errors that are supposed to be added to the bid response are added to the debug log data response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. Pretty good catch


// 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 recordImpMetrics(bidRequest *openrtb.BidRequest, metricsEngine pbsmetrics.MetricsEngine) {

SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
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,
}
metricsEngine.RecordImps(impLabels)
}

}

type DealTierInfo struct {
Prefix string `json:"prefix"`
MinDealTier int `json:"minDealTier"`
Expand Down Expand Up @@ -479,6 +483,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 +505,19 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_

bidResponse.SeatBid = seatBids

if bidResponseExt == nil {
contextDebugValue := ctx.Value(DebugContextKey)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
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