Simplifying exchange module: bidResponseExt gets built anyway #1518
Conversation
| debugLog.Data.Response = "Unable to marshal response ext for debugging" | ||
| errs = append(errs, err) | ||
| } | ||
| if !anyBidsReturned { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| debugLog.Data.Response = "Unable to marshal response ext for debugging" | ||
| errs = append(errs, err) | ||
| } | ||
| if !anyBidsReturned { |
There was a problem hiding this comment.
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.
exchange/exchange.go
Outdated
| // 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...) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Make the ext bid response:
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs) - Set any cache errors on the bid response:
bidResponseExt.Errors[openrtb_ext.PrebidExtKey] = append(bidResponseExt.Errors[openrtb_ext.PrebidExtKey], bidderCacheErrs...) - 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.
There was a problem hiding this comment.
Corrected. Pretty good catch
According to the current logic in
exchange/exchange.go, functionmakeExtBidResponse(adapterBids, adapterExtra, bidRequest, debugInfo, errs)gets called under every scenario either fromHoldAuction()or frombuildBidResponse(). This opens the possibility of slightly simplifying these couple of functions. Please let me know what you think of this small refactoring PR.