-
Notifications
You must be signed in to change notification settings - Fork 768
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
New Adapter: VideoByte #2058
New Adapter: VideoByte #2058
Conversation
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
adapters/videobyte/videobyte.go
Outdated
"github.com/mxmCherry/openrtb/v15/openrtb2" | ||
) | ||
|
||
type VideoByteAdapter struct { |
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.
Please rename this to adapter
. VideoByte
in the name is redundant as the package name already contains videoByte
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.
Renamed
adapters/videobyte/videobyte.go
Outdated
func (a *VideoByteAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
impressions := request.Imp | ||
adapterRequests := make([]*adapters.RequestData, 0, len(impressions)) | ||
errs := make([]error, 0, len(impressions)) |
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.
Please remove the capacity for the errs
slice here. Specifying capacity will lead to Go creating an underlying array of that length in memory thus holding up that memory. This makes sense for adapterRequests
because you expect the number of adapter requests to be equal to the number of impressions but for errors, we ideally expect it to be 0 and so we don't need to reserve memory for errors equal to number of impressions.
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.
Changed it so the slice is initially nil
|
||
func getMediaTypeForImp(imp *openrtb2.Imp) openrtb_ext.BidType { | ||
if imp != nil && imp.Banner != nil { | ||
return openrtb_ext.BidTypeBanner |
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.
Is your bid server always guaranteed to respond with a banner bid for multi-format imps that may have both banner and video configured? If not, is there any indication in the bid returned about the media type? If so, I'd recommend using that to determine the media type of the bid
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, it is guaranteed to respond with a banner bid for multi-format imps that have both video and banner.
adapters/videobyte/videobyte_test.go
Outdated
) | ||
|
||
func TestJsonSamples(t *testing.T) { | ||
adapterstest.RunJSONBidderTest(t, "videobytetest", &VideoByteAdapter{endpoint: "https://mock.videobyte.com"}) |
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.
Please consider using the Builder
function to get an instance of the adapter to be passed into the RunJSONBidderTest
function so that there's test coverage for that function too.
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.
Done.
errs := make([]error, 0, len(impressions)) | ||
|
||
for _, impression := range impressions { | ||
impExt, err := parseExt(&impression) |
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.
Please consider adding a JSON test with multiple imps and also a JSON test with a multi-format imp.
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.
Added.
params := url.Values{} | ||
params.Add("source", "pbs") | ||
params.Add("pid", impExt.PublisherId) | ||
if impExt.PlacementId != "" { |
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.
Please consider including empty placement ID and network ID in one of your JSON test cases
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.
Added a test case.
headers.Add("Accept", "application/json") | ||
|
||
if request.Site != nil { | ||
if request.Site.Domain != "" { |
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.
Please consider including empty Site.Domain and site.Ref in one of your JSON test cases
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.
Added a test case.
* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...
No description provided.