-
Notifications
You must be signed in to change notification settings - Fork 735
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
Debug disable feature implementation: #1677
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a5886cd
Debug disable feature implementation:
2e05c58
Unit tests fix
4c7d53a
Possible cache fix
c7ca474
Added unit tests
f485731
Code review fixe: removed account level debug variable from context
9821fd3
Code review fix
5f1e090
Moved bidder level debug struct to adapters/info.go
fdbe72f
Code review fixes
6c64aa8
Cache fix
d40309a
Minor refactoring
e482baf
Code refactoring and bugfix
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,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 currency.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) | ||
requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo, accountDebugAllowed bool) (*pbsOrtbSeatBid, []error) | ||
} | ||
|
||
// pbsOrtbBid is a Bid returned by an adaptedBidder. | ||
|
@@ -93,7 +93,7 @@ type pbsOrtbSeatBid struct { | |
// | ||
// The name refers to the "Adapter" architecture pattern, and should not be confused with a Prebid "Adapter" | ||
// (which is being phased out and replaced by Bidder for OpenRTB auctions) | ||
func adaptBidder(bidder adapters.Bidder, client *http.Client, cfg *config.Configuration, me metrics.MetricsEngine, name openrtb_ext.BidderName) adaptedBidder { | ||
func adaptBidder(bidder adapters.Bidder, client *http.Client, cfg *config.Configuration, me metrics.MetricsEngine, name openrtb_ext.BidderName, debugInfo *adapters.DebugInfo) adaptedBidder { | ||
return &bidderAdapter{ | ||
Bidder: bidder, | ||
BidderName: name, | ||
|
@@ -102,10 +102,18 @@ func adaptBidder(bidder adapters.Bidder, client *http.Client, cfg *config.Config | |
config: bidderAdapterConfig{ | ||
Debug: cfg.Debug, | ||
DisableConnMetrics: cfg.Metrics.Disabled.AdapterConnectionMetrics, | ||
DebugInfo: adapters.DebugInfo{Allow: parseDebugInfo(debugInfo)}, | ||
}, | ||
} | ||
} | ||
|
||
func parseDebugInfo(info *adapters.DebugInfo) bool { | ||
if info == nil { | ||
return true | ||
} | ||
return info.Allow | ||
} | ||
|
||
type bidderAdapter struct { | ||
Bidder adapters.Bidder | ||
BidderName openrtb_ext.BidderName | ||
|
@@ -117,9 +125,10 @@ type bidderAdapter struct { | |
type bidderAdapterConfig struct { | ||
Debug config.Debug | ||
DisableConnMetrics bool | ||
DebugInfo adapters.DebugInfo | ||
} | ||
|
||
func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) { | ||
func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo, accountDebugAllowed bool) (*pbsOrtbSeatBid, []error) { | ||
reqData, errs := bidder.Bidder.MakeRequests(request, reqInfo) | ||
|
||
if len(reqData) == 0 { | ||
|
@@ -155,8 +164,14 @@ 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 debugInfo := ctx.Value(DebugContextKey); debugInfo != nil && debugInfo.(bool) { | ||
seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo)) | ||
// Write debug data to ext in case if: | ||
// - debugContextKey (url param) in true | ||
// - account debug is allowed | ||
// - bidder debug is allowed | ||
if accountDebugAllowed && bidder.config.DebugInfo.Allow { | ||
if debugInfo := ctx.Value(DebugContextKey); debugInfo != nil && debugInfo.(bool) { | ||
seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo)) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you combine the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, was going to to this too, done! :) |
||
} | ||
|
||
if httpInfo.err == nil { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should debug be allowed by default? If so, perhaps this should be a pointer so we can detect when it's missing or renamed (e.g. Disallow) so that the default value of false is appropriate when the field is missing?
Edit: I see @hhhjort mentioned this in another comment. Technically this is ok right now because
Allow
is the only field inDebugInfo
so we could go with the implementation here where we rely on checking for the presence ofDebug
. However, it's possible we add additional fields toDebugInfo
in the future, in which case we probably don't want to solely rely on checking for the presence ofDebug
to determine if we need to setAllow
to the default. Maybe we don't care about that right now and will just refactor in the future if additional fields are added toDebugInfo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The entire
DebugInfo
structure is a pointer currently. I think that's fine.As you said, another option is to make the
Debug
field inBidderInfo
not a pointer and add a pointer here. That would make it more future proof, but the current approach solves the problem at hand. tomato/tomato IMHO.