-
Notifications
You must be signed in to change notification settings - Fork 769
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
Ext Fields Exposed to Bidders #2331
Conversation
@@ -48,7 +48,7 @@ type bidExt struct { | |||
} | |||
|
|||
type bidTtxExt struct { | |||
MediaType string `json:mediaType,omitempty` | |||
MediaType string `json:"mediaType,omitempty"` |
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.
Unrelated issue discovered during this PR.
"districtm": "appnexus" | ||
} | ||
} | ||
"prebid": {} |
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.
There is an existing behavior where if ext.prebid
is empty the empty object is written to the request. It's easy to remove this field instead. Is that safe? I imagine the current functionality is an accident, but it does provide a signal downstream that the request was processed by PBS. Do you think it's important to preserve that behavior? I'd personally love to make the request as compact/small as possible.
} | ||
} | ||
}], | ||
], |
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.
I used a different json formatter, so there will be some of these minor changes. The VS Code Beautify plugin is no longer maintained.
|
||
CurrencyConversions *ExtRequestCurrency `json:"currency,omitempty"` | ||
BidderConfigs []BidderConfig `json:"bidderconfig,omitempty"` | ||
Experiment *Experiment `json:"experiment,omitempty"` |
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.
I alphabetized the list for easy of reading, except for NoSale since it has a length commend which breaks the code flow.
return false | ||
} | ||
return true | ||
return reqExt != nil && reqExt.Prebid.SChains != nil |
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.
Simplifying while I came across it.
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.
LGTM
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.
Looks good. Just a couple of comments.
Also I see on the corresponding issue that the following fields are also permitted:
ext.prebid.interstitial
ext.prebid.multibid (only their value)
ext.prebid.auctiontimestamp
I don't see any evidence that we support these at the moment, however, I do see that multibid is currently being worked on.
exchange/utils.go
Outdated
sanitizedImpExt := make(map[string]json.RawMessage, 8) | ||
sanitizedImpPrebidExt := make(map[string]json.RawMessage, 8) |
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.
Shouldn't the size of these be 6 and 2 respectively instead of 8 and 8?
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
exchange/utils.go
Outdated
sanitizedImpExt[openrtb_ext.PrebidExtKey] = impExtPrebidJSON | ||
} else { | ||
return nil, fmt.Errorf("cannot marshal ext.prebid: %v", err) | ||
} | ||
} | ||
|
||
// copy reserved imp[].ext fields known to bot be bidder names |
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.
Super nitpick: typo "bot be"
|
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.
LGTM
* 'master' of https://github.com/wwwyyy/prebid-server: Add account-specific error metrics to setuid endpoint (prebid#2337) Add iframe usersync support for ix (prebid#2328) Ext Fields Exposed to Bidders (prebid#2331) Add redirect sync to richaudienceAdapter (prebid#2343)
Resolves #2321