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

Separate "debug" behavior from "billable" behavior #1387

Merged
merged 17 commits into from
Aug 6, 2020
Merged

Separate "debug" behavior from "billable" behavior #1387

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
525f4b7
first draft
Jul 9, 2020
2a44f47
Feedback corrections. Removed unnecessary test case
Jul 15, 2020
ea1b013
Update with latest master
Jul 15, 2020
200abc1
Scott's review feedback
Jul 16, 2020
566417e
Removed boolean argument from requestBid
Jul 16, 2020
9515325
corrected buildBidRequest
Jul 16, 2020
30d01d4
removed anoying jsonpatch tag from imports again
Jul 16, 2020
47d75d4
Stepping out real quick
Jul 22, 2020
cc85f02
prepareExt function tweak
Jul 23, 2020
abe0bbe
Added TestExtractBidRequesteExtInfo and Mansi's feedback
Jul 23, 2020
8bd1af4
Merged latest master updates and resolved conflicts
Jul 23, 2020
0be0124
Reviews feedback
Jul 29, 2020
86a2634
Getting rid of resolvedRequest
Jul 30, 2020
ed5044a
Second half of Mansi's review corrections
Aug 3, 2020
210f0a6
update with latest master
Aug 3, 2020
a27d8a2
Mansi's August 4th review
Aug 5, 2020
c54ffbe
Mansi's Aug 6th review
Aug 6, 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
6 changes: 3 additions & 3 deletions exchange/bidder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type adaptedBidder interface {
//
// Any errors will be user-facing in the API.
// Error messages should help publishers understand what might account for "bad" bids.
requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error)
requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo, debugMode bool) (*pbsOrtbSeatBid, []error)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function signature is becoming bigger and bigger every day lol.

How about utilizing ctx to store request level info like debugMode? You can then just add this value to ctx in the HoldAuction function and then can read it out from ctx in subsequent functions like getAllBids, requestBid, etc without having to pass them around in the function arguments. You can use the context.WithValue function to achieve this.

WithValue provides a way to associate request-scoped values with a Context:

// WithValue returns a copy of parent whose Value method returns val for key.
func WithValue(parent Context, key interface{}, val interface{}) Context

Ref: https://blog.golang.org/context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It did simplify the PR Mansi, thanks

}

// pbsOrtbBid is a Bid returned by an adaptedBidder.
Expand Down Expand Up @@ -103,7 +103,7 @@ type bidderAdapter struct {
me pbsmetrics.MetricsEngine
}

func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) {
func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo, debugMode bool) (*pbsOrtbSeatBid, []error) {
reqData, errs := bidder.Bidder.MakeRequests(request, reqInfo)

if len(reqData) == 0 {
Expand Down Expand Up @@ -139,7 +139,7 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.Bi
for i := 0; i < len(reqData); i++ {
httpInfo := <-responseChannel
// If this is a test bid, capture debugging info from the requests.
if request.Test == 1 {
if debugMode {
seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo))
}

Expand Down
11 changes: 8 additions & 3 deletions exchange/bidder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestSingleBidder(t *testing.T) {
}
bidder := adaptBidder(bidderImpl, server.Client(), &config.Configuration{}, &metricsConfig.DummyMetricsEngine{})
currencyConverter := currencies.NewRateConverterDefault()
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", bidAdjustment, currencyConverter.Rates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", bidAdjustment, currencyConverter.Rates(), &adapters.ExtraRequestInfo{}, false)

// Make sure the goodSingleBidder was called with the expected arguments.
if bidderImpl.httpResponse == nil {
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestMultiBidder(t *testing.T) {
}
bidder := adaptBidder(bidderImpl, server.Client(), &config.Configuration{}, &metricsConfig.DummyMetricsEngine{})
currencyConverter := currencies.NewRateConverterDefault()
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", 1.0, currencyConverter.Rates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", 1.0, currencyConverter.Rates(), &adapters.ExtraRequestInfo{}, false)

if seatBid == nil {
t.Fatalf("SeatBid should exist, because bids exist.")
Expand Down Expand Up @@ -526,6 +526,7 @@ func TestMultiCurrencies(t *testing.T) {
1,
currencyConverter.Rates(),
&adapters.ExtraRequestInfo{},
false,
)

// Verify:
Expand Down Expand Up @@ -670,6 +671,7 @@ func TestMultiCurrencies_RateConverterNotSet(t *testing.T) {
1,
currencyConverter.Rates(),
&adapters.ExtraRequestInfo{},
false,
)

// Verify:
Expand Down Expand Up @@ -843,6 +845,7 @@ func TestMultiCurrencies_RequestCurrencyPick(t *testing.T) {
1,
currencyConverter.Rates(),
&adapters.ExtraRequestInfo{},
false,
)

// Verify:
Expand Down Expand Up @@ -955,6 +958,7 @@ func TestServerCallDebugging(t *testing.T) {
1.0,
currencyConverter.Rates(),
&adapters.ExtraRequestInfo{},
true,
)

if len(bids.httpCalls) != 1 {
Expand Down Expand Up @@ -1065,6 +1069,7 @@ func TestMobileNativeTypes(t *testing.T) {
1.0,
currencyConverter.Rates(),
&adapters.ExtraRequestInfo{},
false,
)

var actualValue string
Expand All @@ -1078,7 +1083,7 @@ func TestMobileNativeTypes(t *testing.T) {
func TestErrorReporting(t *testing.T) {
bidder := adaptBidder(&bidRejector{}, nil, &config.Configuration{}, &metricsConfig.DummyMetricsEngine{})
currencyConverter := currencies.NewRateConverterDefault()
bids, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", 1.0, currencyConverter.Rates(), &adapters.ExtraRequestInfo{})
bids, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, "test", 1.0, currencyConverter.Rates(), &adapters.ExtraRequestInfo{}, false)
if bids != nil {
t.Errorf("There should be no seatbid if no http requests are returned.")
}
Expand Down
4 changes: 2 additions & 2 deletions exchange/bidder_validate_bids.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ type validatedBidder struct {
bidder adaptedBidder
}

func (v *validatedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) {
seatBid, errs := v.bidder.requestBid(ctx, request, name, bidAdjustment, conversions, reqInfo)
func (v *validatedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo, debugMode bool) (*pbsOrtbSeatBid, []error) {
seatBid, errs := v.bidder.requestBid(ctx, request, name, bidAdjustment, conversions, reqInfo, false)
if validationErrors := removeInvalidBids(request, seatBid); len(validationErrors) > 0 {
errs = append(errs, validationErrors...)
}
Expand Down
10 changes: 5 additions & 5 deletions exchange/bidder_validate_bids_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestAllValidBids(t *testing.T) {
},
},
})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{}, false)
assert.Len(t, seatBid.bids, 3)
assert.Len(t, errs, 0)
}
Expand Down Expand Up @@ -83,7 +83,7 @@ func TestAllBadBids(t *testing.T) {
},
},
})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{}, false)
assert.Len(t, seatBid.bids, 0)
assert.Len(t, errs, 5)
}
Expand Down Expand Up @@ -126,7 +126,7 @@ func TestMixedBids(t *testing.T) {
},
},
})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{}, false)
assert.Len(t, seatBid.bids, 2)
assert.Len(t, errs, 3)
}
Expand Down Expand Up @@ -246,7 +246,7 @@ func TestCurrencyBids(t *testing.T) {
Cur: tc.brqCur,
}

seatBid, errs := bidder.requestBid(context.Background(), request, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{})
seatBid, errs := bidder.requestBid(context.Background(), request, openrtb_ext.BidderAppnexus, 1.0, currencies.NewConstantRates(), &adapters.ExtraRequestInfo{}, false)
assert.Len(t, seatBid.bids, expectedValidBids)
assert.Len(t, errs, expectedErrs)
}
Expand All @@ -257,6 +257,6 @@ type mockAdaptedBidder struct {
errorResponse []error
}

func (b *mockAdaptedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) {
func (b *mockAdaptedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo, debugMode bool) (*pbsOrtbSeatBid, []error) {
return b.bidResponse, b.errorResponse
}
23 changes: 13 additions & 10 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
shouldCacheVAST := false
var bidAdjustmentFactors map[string]float64
var requestExt openrtb_ext.ExtRequest
debugInfo := bidRequest.Test == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider calling this debugMode to match with the getAllBids argument name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if len(bidRequest.Ext) > 0 {
err := json.Unmarshal(bidRequest.Ext, &requestExt)
if err != nil {
Expand All @@ -139,6 +140,8 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
}
targData.cacheHost, targData.cachePath = e.cache.GetExtCacheData()
}
// if true, the bidResponse will be returned with debug information.
debugInfo = debugInfo || requestExt.Prebid.Debug
}

// If we need to cache bids, then it will take some time to call prebid cache.
Expand All @@ -149,7 +152,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
// Get currency rates conversions for the auction
conversions := e.currencyConverter.Rates()

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

var auc *auction = nil
var bidResponseExt *openrtb_ext.ExtBidResponse = nil
Expand Down Expand Up @@ -180,7 +183,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
}

if debugLog != nil && debugLog.Enabled {
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, resolvedRequest, errs)
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, resolvedRequest, debugInfo, errs)
if bidRespExtBytes, err := json.Marshal(bidResponseExt); err == nil {
debugLog.Data.Response = string(bidRespExtBytes)
} else {
Expand All @@ -205,7 +208,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
}

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

type DealTierInfo struct {
Expand Down Expand Up @@ -296,7 +299,7 @@ func (e *exchange) makeAuctionContext(ctx context.Context, needsCache bool) (auc
}

// This piece sends all the requests to the bidder adapters and gathers the results.
func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra, bool) {
func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions, debugInfo bool) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra, bool) {
// Set up pointers to the bid results
adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
Expand Down Expand Up @@ -326,7 +329,7 @@ func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext
}
var reqInfo adapters.ExtraRequestInfo
reqInfo.PbsEntryPoint = bidlabels.RType
bids, err := e.adapterMap[coreBidder].requestBid(ctx, request, aName, adjustmentFactor, conversions, &reqInfo)
bids, err := e.adapterMap[coreBidder].requestBid(ctx, request, aName, adjustmentFactor, conversions, &reqInfo, debugInfo)

// Add in time reporting
elapsed := time.Since(start)
Expand Down Expand Up @@ -445,7 +448,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, resolvedRequest json.RawMessage, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, errList []error) (*openrtb.BidResponse, error) {
func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb.BidRequest, resolvedRequest json.RawMessage, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, debugInfo bool, errList []error) (*openrtb.BidResponse, error) {
bidResponse := new(openrtb.BidResponse)

bidResponse.ID = bidRequest.ID
Expand All @@ -469,7 +472,7 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
bidResponse.SeatBid = seatBids

if bidResponseExt == nil {
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, resolvedRequest, errList)
bidResponseExt = e.makeExtBidResponse(adapterBids, adapterExtra, bidRequest, resolvedRequest, debugInfo, errList)
}
buffer := &bytes.Buffer{}
enc := json.NewEncoder(buffer)
Expand Down Expand Up @@ -656,13 +659,13 @@ func getPrimaryAdServer(adServerId int) (string, error) {
}

// Extract all the data from the SeatBids and build the ExtBidResponse
func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, req *openrtb.BidRequest, resolvedRequest json.RawMessage, errList []error) *openrtb_ext.ExtBidResponse {
func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, req *openrtb.BidRequest, resolvedRequest json.RawMessage, debugInfo bool, errList []error) *openrtb_ext.ExtBidResponse {
bidResponseExt := &openrtb_ext.ExtBidResponse{
Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderError, len(adapterBids)),
ResponseTimeMillis: make(map[openrtb_ext.BidderName]int, len(adapterBids)),
RequestTimeoutMillis: req.TMax,
}
if req.Test == 1 {
if debugInfo {
bidResponseExt.Debug = &openrtb_ext.ExtResponseDebug{
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is not a change specific to this PR but I'd suggest we call buildResolvedRequest here itself if debugInfo is set rather than calling it upstream and passing it down because this is the only function that actually uses it.

This should be a simple and quick enough simplification of the code.

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'd agree with you if HoldAuction's second argument bidRequest *openrtb.BidRequest does not get modified anywhere from exchange/exchange.go's line 100 all the way here. Let me double check because it'll be great if we could get rid of this extra argument in functions makeExtBidResponse and buildBidResponse

HttpCalls: make(map[openrtb_ext.BidderName][]*openrtb_ext.ExtHttpCall),
}
Expand All @@ -673,7 +676,7 @@ func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pb

for bidderName, responseExtra := range adapterExtra {

if req.Test == 1 {
if debugInfo {
bidResponseExt.Debug.HttpCalls[bidderName] = responseExtra.HttpCalls
}
// Only make an entry for bidder errors if the bidder reported any.
Expand Down
Loading