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

Debug disable feature implementation: #1677

Merged
merged 11 commits into from
Jan 28, 2021
Merged

Debug disable feature implementation: #1677

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a5886cd
Debug disable feature implementation:
Jan 22, 2021
2e05c58
Unit tests fix
Jan 22, 2021
4c7d53a
Possible cache fix
Jan 23, 2021
c7ca474
Added unit tests
Jan 25, 2021
f485731
Code review fixe: removed account level debug variable from context
Jan 26, 2021
9821fd3
Code review fix
Jan 26, 2021
5f1e090
Moved bidder level debug struct to adapters/info.go
Jan 26, 2021
fdbe72f
Code review fixes
Jan 26, 2021
6c64aa8
Cache fix
Jan 26, 2021
d40309a
Minor refactoring
Jan 27, 2021
e482baf
Code refactoring and bugfix
Jan 28, 2021
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
5 changes: 5 additions & 0 deletions adapters/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ type BidderInfo struct {
Capabilities *CapabilitiesInfo `yaml:"capabilities" json:"capabilities"`
AliasOf string `json:"aliasOf,omitempty"`
ModifyingVastXmlAllowed bool `yaml:"modifyingVastXmlAllowed" json:"-" xml:"-"`
Debug *DebugInfo `yaml:"debug,omitempty" json:"-" xml:"-"`
}

type DebugInfo struct {
Allow bool `yaml:"allow" json:"allow"`
Copy link
Collaborator

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 in DebugInfo so we could go with the implementation here where we rely on checking for the presence of Debug. However, it's possible we add additional fields to DebugInfo in the future, in which case we probably don't want to solely rely on checking for the presence of Debug to determine if we need to set Allow to the default. Maybe we don't care about that right now and will just refactor in the future if additional fields are added to DebugInfo.

Copy link
Contributor

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?

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 in BidderInfo 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.

}

type MaintainerInfo struct {
Expand Down
1 change: 1 addition & 0 deletions config/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Account struct {
EventsEnabled bool `mapstructure:"events_enabled" json:"events_enabled"`
CCPA AccountCCPA `mapstructure:"ccpa" json:"ccpa"`
GDPR AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
DebugAllowed bool `mapstructure:"debug_allowed" json:"debug_allowed"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to my comment above in the bidder config, should debug be allowed by default? Perhaps a pointer or rename if the default should be to allow?

Edit: on second thought, this is ok since debug_allowed is set to true in the account defaults and that gets merged with any ingested account yaml.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be DebugAllow / debug_allow per the spec and to coordinate with debug.allow in the bidder info.

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

}

// AccountCCPA represents account-specific CCPA configuration
Expand Down
1 change: 1 addition & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("blacklisted_accts", []string{""})
v.SetDefault("account_required", false)
v.SetDefault("account_defaults.disabled", false)
v.SetDefault("account_defaults.debug_allowed", true)
v.SetDefault("certificates_file", "")
v.SetDefault("auto_gen_source_tid", true)

Expand Down
2 changes: 1 addition & 1 deletion exchange/adapter_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func buildExchangeBidders(cfg *config.Configuration, infos adapters.BidderInfos,

exchangeBidders := make(map[openrtb_ext.BidderName]adaptedBidder, len(bidders))
for bidderName, bidder := range bidders {
exchangeBidders[bidderName] = adaptBidder(bidder, client, cfg, me, bidderName)
exchangeBidders[bidderName] = adaptBidder(bidder, client, cfg, me, bidderName, infos[bidderName.String()].Debug)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a guarantee that infos[bidderName.String()] will be found, but only because there's a check in buildBidders. I don't think that relationship is obvious enough. I recommend to add protections for the lookup here as well. Something like:

for bidderName, bidder := range bidders {
  info, infoFound := infos[string(bidderName)]
  if !infoFound {
    errs = append(errs, fmt.Errorf("%v: bidder info not found", bidder))
    continue
  }

  exchangeBidders[bidderName] = adaptBidder(bidder, client, cfg, me, bidderName, info.Debug)
}

return exchangeBidders, errs

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!

}

return exchangeBidders, nil
Expand Down
8 changes: 4 additions & 4 deletions exchange/adapter_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestBuildAdaptersSuccess(t *testing.T) {

appnexusBidder, _ := appnexus.Builder(openrtb_ext.BidderAppnexus, config.Adapter{})
appnexusBidderWithInfo := adapters.EnforceBidderInfo(appnexusBidder, infoActive)
appnexusBidderAdapted := adaptBidder(appnexusBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderAppnexus)
appnexusBidderAdapted := adaptBidder(appnexusBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderAppnexus, nil)
appnexusBidderValidated := addValidatedBidderMiddleware(appnexusBidderAdapted)

idLegacyAdapted := &adaptedAdapter{lifestreet.NewLifestreetLegacyAdapter(adapters.DefaultHTTPAdapterConfig, "anyEndpoint")}
Expand Down Expand Up @@ -78,11 +78,11 @@ func TestBuildExchangeBidders(t *testing.T) {

appnexusBidder, _ := appnexus.Builder(openrtb_ext.BidderAppnexus, config.Adapter{})
appnexusBidderWithInfo := adapters.EnforceBidderInfo(appnexusBidder, infoActive)
appnexusBidderAdapted := adaptBidder(appnexusBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderAppnexus)
appnexusBidderAdapted := adaptBidder(appnexusBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderAppnexus, nil)

rubiconBidder, _ := rubicon.Builder(openrtb_ext.BidderRubicon, config.Adapter{})
rubiconBidderWithInfo := adapters.EnforceBidderInfo(rubiconBidder, infoActive)
rubiconBidderAdapted := adaptBidder(rubiconBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderRubicon)
rubiconBidderAdapted := adaptBidder(rubiconBidderWithInfo, client, &config.Configuration{}, metricEngine, openrtb_ext.BidderRubicon, nil)

testCases := []struct {
description string
Expand Down Expand Up @@ -405,7 +405,7 @@ func TestGetDisabledBiddersErrorMessages(t *testing.T) {

type fakeAdaptedBidder struct{}

func (fakeAdaptedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) {
func (fakeAdaptedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo, accountDebugAllowed bool) (*pbsOrtbSeatBid, []error) {
return nil, nil
}

Expand Down
25 changes: 21 additions & 4 deletions exchange/bidder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -155,8 +164,16 @@ 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.
// Write debug data to ext in case if:
// - debugContextKey (url param) in true
// - account debug is allowed
// - bidder debug is allowed
if debugInfo := ctx.Value(DebugContextKey); debugInfo != nil && debugInfo.(bool) {
seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo))
if accountDebugAllowed {
if bidder.config.DebugInfo.Allow {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we need debug enabled at both the bidder and the account level to see debug information. If that's the case, do we need the bidder level setting? Maybe we're requiring it as well as a safeguard...

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In case we bid more than one bidder in one request, some of those bidders may have debug.allowed=true and others may have debug.allowed=false. For this case we need to make 2 checks(account and bidder level) and write logs for those bidders where debug.allowed=true. Hope this make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait... did my comment disappear?
Correct. If account level debug disabled, we don't need to check bidder level config. We just skip it.
But if account debug is allowed, we need to check every bidder in request.

We may bid more than one bidder in one request and those bidders may have different debug options. To handle this case we need to check each of them and log data only for those bidders where debug.allow = true.

Hope this makes sense and answers your question.

seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you combine the if clauses to improve readability?

if accountDebugAllowed && bidder.config.DebugInfo.Allow {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, was going to to this too, done! :)

}

if httpInfo.err == nil {
Expand Down
Loading