-
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
New Adapter: FeedAd #4104
New Adapter: FeedAd #4104
Conversation
Code coverage summaryNote:
feedadRefer here for heat map coverage report
|
headers.Add("Accept", "application/json") | ||
headers.Add("Content-Type", "application/json;charset=utf-8") | ||
headers.Add("X-FA-PBS-Adapter-Version", feedAdAdapterVersion) | ||
headers.Add("X-Openrtb-Version", "2.5") |
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.
Are you able to support OpenRTB 2.6 instead?
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’m afraid, we don’t have support for oRTB 2.6, yet, but it’s on our roadmap for next year.
Can we please start with oRTB 2.5 for now?
Code coverage summaryNote:
feedadRefer here for heat map coverage report
|
@Rothalack can you please review? |
I've sent a verification email to your maintainer email. |
Hi @Rothalack , you should have received a response to your confirmation email. |
I got the response, thank you! |
adapters/feedad/params_test.go
Outdated
var invalidParams = []string{ | ||
`{}`, | ||
`{"clientToken":"","placementId":"some-placementid"}`, | ||
`{"clientToken":"some-clienttoken","placementId":""}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":"complete-garbage"}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"advertising_id":{}}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"advertising_id":{}}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"app_name":{}}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"bundle_id":{}}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"hybrid_platform":{}}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"limit_ad_tracking":{}}}`, |
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 suggest adding some more invalid param test cases to ensure that your regex for placementId
is correct.
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.
✅
adapters/feedad/params_test.go
Outdated
var validParams = []string{ | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid"}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"hybrid_platform":"ios"}}`, | ||
`{"clientToken":"some-clienttoken","placementId":"some-placementid","sdkOptions":{"hybrid_platform":"windows"}}`, | ||
`{"clientToken":"some-clienttoken","decoration":"some-decoration","placementId":"some-placementid","sdkOptions":{"advertising_id":"","app_name":"","bundle_id":"","hybrid_app":false,"hybrid_platform":"","limit_ad_tracking":false}}`, | ||
`{"clientToken":"some-clienttoken","decoration":"some-decoration","placementId":"some-placementid","sdkOptions":{"advertising_id":"some-advertisingid","app_name":"some-appname","bundle_id":"some-bundleid","hybrid_app":true,"hybrid_platform":"android","limit_ad_tracking":true}}`, | ||
} |
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 suggest adding some more valid param test cases to ensure that your regex for placementId
is correct.
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.
✅
@@ -0,0 +1,17 @@ | |||
endpoint: "https://ortb.feedad.com/1/prebid/requests" |
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.
Verified endpoint is reachable:
curl -i --location --request POST https://ortb.feedad.com/1/prebid/requests
HTTP/2 400
cache-control: private, no-cache, no-store
content-type: text/plain; charset=utf-8
x-cloud-trace-context: 1330f4943e1c24fead2cd0c791a59b3d
date: Tue, 28 Jan 2025 19:35:58 GMT
server: Google Frontend
content-length: 15
via: 1.1 google
alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
400 Bad Request
endpointCompression: gzip | ||
maintainer: | ||
email: support@feedad.com | ||
gvlVendorID: 781 |
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.
Verified GVL ID
curl https://vendor-list.consensu.org/v3/vendor-list.json | jq '.vendors."781"'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 658k 100 658k 0 0 3647k 0 --:--:-- --:--:-- --:--:-- 3640k
{
"id": 781,
"name": "FeedAd GmbH",
"purposes": [
1,
3,
4,
5,
6,
8,
9
],
"legIntPurposes": [
2,
7,
10
],
"flexiblePurposes": [
2,
7,
10
],
"specialPurposes": [
1,
2,
3
],
"features": [
2,
3
],
"specialFeatures": [
1,
2
],
"overflow": {
"httpGetLimit": 128
},
"cookieMaxAgeSeconds": 2592000,
"usesCookies": true,
"cookieRefresh": true,
"urls": [
{
"langId": "en",
"privacy": "https://feedad.com/privacy/",
"legIntClaim": "https://feedad.com/privacy/#legal-basis"
}
],
"usesNonCookieAccess": true,
"dataRetention": {
"stdRetention": 30,
"purposes": {},
"specialPurposes": {
"3": 4320
}
},
"dataDeclaration": [
1,
2,
3,
6,
7,
8,
9,
10,
11
],
"deviceStorageDisclosureUrl": "https://api.feedad.com/tcf-device-disclosures.json"
}
iframe: | ||
url: https://ortb.feedad.com/1/usersyncs/supply?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&gpp={{.GPP}}&gpp_sid={{.GPPSID}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} | ||
userMacro: $UID |
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'm getting a 404 using this URL generated by my localhost cookie_sync endpoint: https://ortb.feedad.com/1/usersyncs/supply?gdpr=0&gdpr_consent=CQL_D2nQL_D2nPjAAAENCZCAAP_AAH_AAAAAGGwAQGGgYbABAYaAAA.II7Nd_X__bX9n-_7_6ft0eY1f9_r37uQzDhfNs-8F3L_W_LwX32E7NF36tq4KmR4ku1bBIQNtHMnUDUmxaolVrzHsak2cpyNKJ_JkknsZe2dYGF9Pn9lD-YKZ7_5_9_f52T_9_9_-39z3_9f___dv_-__-vjf_599n_v9fV_78_Kf9______-____________8A&gpp=&gpp_sid=&us_privacy=&redirect=http%3A%2F%2Flocalhost%3A8000%2Fsetuid%3Fbidder%3Dfeedad%26gdpr%3D0%26gdpr_consent%3DCQL_D2nQL_D2nPjAAAENCZCAAP_AAH_AAAAAGGwAQGGgYbABAYaAAA.II7Nd_X__bX9n-_7_6ft0eY1f9_r37uQzDhfNs-8F3L_W_LwX32E7NF36tq4KmR4ku1bBIQNtHMnUDUmxaolVrzHsak2cpyNKJ_JkknsZe2dYGF9Pn9lD-YKZ7_5_9_f52T_9_9_-39z3_9f___dv_-__-vjf_599n_v9fV_78_Kf9______-____________8A%26gpp%3D%26gpp_sid%3D%26f%3Db%26uid%3D%24UID
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, actually you should be getting a 400 Bad Request, since we only allow the scheme https
in redirect urls.
Quick curl example with example.com:
curl -i --location 'https://ortb.feedad.com/1/usersyncs/supply?gdpr=0&gdpr_consent=CQL_D2nQL_D2nPjAAAENCZCAAP_AAH_AAAAAGGwAQGGgYbABAYaAAA.II7Nd_X__bX9n-_7_6ft0eY1f9_r37uQzDhfNs-8F3L_W_LwX32E7NF36tq4KmR4ku1bBIQNtHMnUDUmxaolVrzHsak2cpyNKJ_JkknsZe2dYGF9Pn9lD-YKZ7_5_9_f52T_9_9_-39z3_9f___dv_-__-vjf_599n_v9fV_78_Kf9______-____________8A&gpp=&gpp_sid=&us_privacy=&redirect=https%3A%2F%2Fexample.com%2F%24UID'
HTTP/2 200
cache-control: private, no-cache, no-store
content-type: text/html; charset=utf-8
x-cloud-trace-context: f012169dfb1e168d52f044ad1f228369
date: Wed, 29 Jan 2025 11:19:14 GMT
server: Google Frontend
content-length: 150
via: 1.1 google
alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
<!DOCTYPE html>
<html lang="en">
<iframe src="https://example.com/23f687c1-f025-434f-b26b-b6773d1b3a5f" style="display: none;"></iframe>
</html>
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.
Verified cookie sync iframe works by sending a request generated by my localhost cookie sync endpoint with https://localhost:8000/setuid
as the redirect param. Received 200 response with iframe html element. Modified the iframe url from https to http and manually triggered that url to verify it hit my localhost setuid endpoint where the cookie was verified to contain the feedad id.



|
||
if request.Device != nil { | ||
if len(request.Device.IPv6) > 0 { | ||
headers.Add("X-Forwarded-For", request.Device.IPv6) |
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 either modify your banner-app.json
or banner-site.json
test to have an ipv6
instead of ip
or add a supplemental JSON test that has ipv6
set to cover this case.
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.
✅
adapters/feedad/feedad.go
Outdated
if responseData.StatusCode == http.StatusNoContent { | ||
return nil, nil | ||
} | ||
|
||
if responseData.StatusCode == http.StatusBadRequest { | ||
err := &errortypes.BadInput{ | ||
Message: "Unexpected status code: 400. Bad request from publisher. Run with request.debug = 1 for more info.", | ||
} | ||
return nil, []error{err} | ||
} | ||
|
||
if responseData.StatusCode != http.StatusOK { | ||
err := &errortypes.BadServerResponse{ | ||
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode), | ||
} | ||
return nil, []error{err} | ||
} |
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 understand that the prebid docs do not reflect this yet but please use the adapter utility functions adapters/response.go#IsResponseStatusCodeNoContent
and adapters/response.go#CheckResponseStatusCodeForErrors
to handle invalid status codes.
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.
✅
adapters/feedad/feedad.go
Outdated
if responseData.StatusCode == http.StatusNoContent { | ||
return nil, nil | ||
} | ||
|
||
if responseData.StatusCode == http.StatusBadRequest { | ||
err := &errortypes.BadInput{ | ||
Message: "Unexpected status code: 400. Bad request from publisher. Run with request.debug = 1 for more info.", | ||
} | ||
return nil, []error{err} | ||
} | ||
|
||
if responseData.StatusCode != http.StatusOK { | ||
err := &errortypes.BadServerResponse{ | ||
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode), | ||
} | ||
return nil, []error{err} | ||
} |
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 add supplemental JSON tests for status codes 204, 400 and 500.
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.
✅
var response openrtb2.BidResponse | ||
err := jsonutil.Unmarshal(responseData.Body, &response) | ||
if err != nil { | ||
return nil, []error{err} |
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 add a supplemental JSON test case where the response is malformed. This can be done by simply setting the mock response body to a string instead of a JSON object.
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.
✅
adapters/feedad/feedad.go
Outdated
} | ||
|
||
requestData := &adapters.RequestData{ | ||
Method: "POST", |
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.
Nitpick: I suggest using http.MethodPost
instead of "POST"
.
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.
✅
… device.ip, added field device.ipv6
…o-device.json test case
…eStatusCodeNoContent,CheckResponseStatusCodeForErrors}()
…0 and 500 responses
Code coverage summaryNote:
feedadRefer here for heat map coverage report
|
Thank you @bsardo for reviewing this PR. |
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!
headers.Add("X-Forwarded-For", request.Device.IPv6) | ||
} | ||
|
||
if len(request.Device.IP) > 0 { |
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.
Under this logic, if a request comes with both root.device.ip
and root.device.ipv6
, ipv4
would get prioritized over ipv6
. Is this convenient? Or should the order of the if
statements be reversed to prioritize ipv6
into the X-Forwarded-For
header if a request with both values comes in?
.
.
"device": {
"ua": "test-user-agent",
"ip": "123.123.123.123",
"ipv6": "2001:db8:3333:4444:5555:6666:7777:8888",
"language": "en",
"dnt": 0
},
.
.
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.
For us, this doesn't really matter.
The only real use case I can think of for looking at the header, would be filtering traffic before it reaches our API.
When processing in the API, we still have both fields ip and ipv6 in the request payload.
Can this really happen that both fields are filled anyway?
Isn't the client IP usually taken from the IP connection the device is connecting to the server?
How can it connect with both IPv4 and IPv6?
Anyway, I don't see a real issue here to be honest.
bidResponse.Bids, | ||
&adapters.TypedBid{ | ||
Bid: &seatBid.Bid[i], | ||
BidType: 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.
Do we expect all of the bids to be banners? Is there a possibility now or in the future that we get Video, Native or Audio? If so, should we grab the type from the response instead of hard-coding the banner value?
57 for _, seatBid := range response.SeatBid {
58 for i := range seatBid.Bid {
+ mediaType, err := getMediaTypeForBid(&seatBid.Bid[i])
+ if err != nil {
+ continue
+ }
59 bidResponse.Bids = append(
60 bidResponse.Bids,
61 &adapters.TypedBid{
62 Bid: &seatBid.Bid[i],
63 - BidType: openrtb_ext.BidTypeBanner,
+ mediaType,
64 },
65 )
66 }
67 }
adapters/feedad/feedad.go
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, we only support banners right now.
See feedad.yaml:
capabilities:
app:
mediaTypes:
- banner
site:
mediaTypes:
- banner
Thank you @guscarreon for reviewing. |
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. Thanks @andwun
This PR adds the FeedAd PBS adapter
Docs PR: prebid/prebid.github.io#5776