-
Notifications
You must be signed in to change notification settings - Fork 762
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
RTBHouse: native support #3212
RTBHouse: native support #3212
Conversation
Code coverage summaryNote:
rtbhouseRefer here for heat map coverage report
|
Code coverage summaryNote:
rtbhouseRefer here for heat map coverage report
|
adapters/rtbhouse/rtbhouse.go
Outdated
default: | ||
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.
should not default to banner type. Should return error if found an unknown bid type
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.
@onkarvhanumante Many adapters behave like that by returning banner type as default and/or do not return an error: see aceex, acuityads,adhese just to name a few from the top and even pubmatic and pubnative.
Our bidder may sometimes skip the mtype
if the response contains banner data. Such a situation indicates we have to suppose we've got banner data in response.
Our Java adapter works also like the above.
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.
@piotrj-rtbh Thanks for the clarifiacation
If bidder skips setting mtype
then can fallback to check if imp.Banner is not null and then return openrtb_ext.BidTypeBanner
Our bidder may sometimes skip the mtype if the response contains banner data
Also should add above as comment in adapter code
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.
If bidder skips setting
mtype
then can fallback to check if imp.Banner is not null and then returnopenrtb_ext.BidTypeBanner
@onkarvhanumante I didn't get it well. Do you mean I need to loop through request's imp[]
array, find matching element (by impid
I suppose) and check it's Banner
prop if it's null or not?
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
If bidder skips setting
mtype
then can fallback to check if imp.Banner is not null and then returnopenrtb_ext.BidTypeBanner
@onkarvhanumante I didn't get it well. Do you mean I need to loop through request's
imp[]
array, find matching element (byimpid
I suppose) and check it'sBanner
prop if it's null or not?
yes for default case to check if banner was set in req.Imp[]
. However this may not work if your adapter support 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.
I think if you wanna default to Banner then you could use the following code to check the native first and then default to banner.
func getMediaTypeForBid(bid openrtb2.Bid) openrtb_ext.BidType {
switch bid.MType {
case openrtb2.MarkupNative:
return openrtb_ext.BidTypeNative
default:
return openrtb_ext.BidTypeBanner
}
The above should work for your adapter. But I strongly recommend that you should change your bidder to properly set MType and not skip that value.
Also mention in the docs that your adapter defaults to 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.
@onkarvhanumante @gargcreation1992
We decided to always return MType
from the bidder. Just in case the bid type detecting function now may return an error if MType
is not supported. Returned bids which have unknown MType
are skipped then.
Code coverage summaryNote:
rtbhouseRefer here for heat map coverage report
|
@piotrj-rtbh , prebid server has published major release 2.0. Requesting to sync with |
Code coverage summaryNote:
rtbhouseRefer here for heat map coverage report
|
@onkarvhanumante Thank you. Fork synced. |
This reverts commit 2540dd8.
@bsardo Could you please describe the reason why this PR commit has been reverted? |
This reverts commit 2540dd8.
Hi @piotrj-rtbh, I apologize for the revert and the slow response. It was solely a matter of circumstance. We built v2.0.0 and then realized there was a problem with it so we put up a fix but our team merged this PR before the patch v2.0.1 was built. As a result, we reverted your PR with the intention of remerging it after the patch was built but forgot to do so. |
Type of change
Description of change
Native ads are now available to be processed and accessed by RTB House adapter. Test coverage has increased significantly.
There is also a little bugfix added to how the adapter handles bid floors. Previous version dropped bid floor defined in a bidder param ("
bidfloor
") when publishers didn't set currency module. Now it is fine with that case and the bidfloor currency is set to the bidder's default (USD).Other information
Please reach us at [email protected] with [email protected] and cc [email protected].