-
Notifications
You must be signed in to change notification settings - Fork 749
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: jixie #1698
New Adapter: jixie #1698
Conversation
…he jixie usersync code
…nner*json test file
adapters/jixie/jixie.go
Outdated
|
||
func buildEndpoint(endpoint string, testing bool, timeout int64) string { | ||
|
||
if timeout == 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.
You want to set the timeout
value outside this function, somewhere in MakeRequests
because arguments are pass by value in Go and so only the timeout
copy local to this method will be updated to the new value and won't be reflected in req.TMax
as you want it to.
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.
tks; tried to fix based on all your comments in 0x1deb9f1 (14 Feb). Sorry I didn't know there is an update slot here.
adapters/jixie/jixie.go
Outdated
var jxExt openrtb_ext.ExtImpJixie | ||
if err := json.Unmarshal(bidderExt.Bidder, &jxExt); err != nil { | ||
return nil, []error{&errortypes.BadInput{ | ||
Message: fmt.Sprintf("ignoring imp id=%s, invalid ImpExt", imp.ID)}, |
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 sure you want to return an error here? It might make more sense to append the error and continue processing the rest of the imps. Same for the bidder ext above.
adapters/jixie/jixie.go
Outdated
|
||
type JixieAdapter struct { | ||
endpoint string | ||
testing bool |
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 like you are using the testing
field only for JSON tests. This is not really needed as the JSON tests won't be hitting your server and so you can remove this field and also the if
block on L43
}], | ||
|
||
"expectedBidResponses": [{ | ||
"bids": [{ |
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 set the currency
field as well here
}], | ||
|
||
"expectedBidResponses": [{ | ||
"bids": [{ |
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.
Same here. Please add the expected currency
as well
adapters/jixie/jixie.go
Outdated
|
||
func (a *JixieAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
var errs []error | ||
if len(request.Imp) == 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.
No need for this check because Prebid Server discards bidRequests with zero-length []Imp
arrays upstream and in consequence, makes the true branch of this if
statement unreachable.
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.
removed in 0xaa2d1d5. tks
adapters/jixie/jixie.go
Outdated
}} | ||
} | ||
|
||
for _, imp := range request.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.
Are we sure we don't want to do anything with the unmarshalled info? Is the purpose of this for
loop is just to check if imps don't have unmarshaling errors?
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.
tks for this great note. Now do more checking; + 2 test json files. (in 0xaa2d1d5)
adapters/jixie/jixie.go
Outdated
addHeaderIfNonEmpty(headers, "Referer", request.Site.Page) | ||
} | ||
|
||
theurl := buildEndpoint(a.endpoint, a.testing, request.TMax) |
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 testing
a left-over from a debug phase?
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.
fixed in earlier commit. tks
adapters/jixie/params_test.go
Outdated
} | ||
|
||
var validParams = []string{ | ||
`{"unit": "1000008-AA77BB88CC", "accountid": "9988776655", "bidfloor": "0.01", "jxprop1": "somethingimportant" }`, |
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.
Can we add a test case where unit
comes on its own?
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. tks ( 0xaa2d1d5)
config/config.go
Outdated
setDefaultUsersync(cfg.Adapters, openrtb_ext.BidderKrushmedia, "https://cs.krushmedia.com/4e4abdd5ecc661643458a730b1aa927d.gif?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redir="+url.QueryEscape(externalURL)+"%2Fsetuid%3Fbidder%3Dkrushmedia%26uid%3D%5BUID%5D") | ||
|
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 this empty line
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.
Removed. tks (in 0xaa2d1d5)
if request.Device.DNT != nil { | ||
addHeaderIfNonEmpty(headers, "DNT", strconv.Itoa(int(*request.Device.DNT))) | ||
} | ||
} |
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.
Consider a test that sets the "Accept-Language"
and "DNT"
the headers:
72 "httpCalls": [{
73 "expectedRequest": {
74 "uri": "https://hb.jixie.io/v2/hbsvrpost?hbofftest=1",
75 "headers": {
76 "Accept": [
77 "application/json"
78 ],
+ "Accept-Language": [
+ "anyAcceptLanguageValue"
+ ],
+ "DNT": [
+ "anyDntValue"
+ ],
79 "Content-Type": [
80 "application/json;charset=utf-8"
81 ],
82 "X-Forwarded-For": [
83 "111.222.333.444"
84 ],
85 "Referer": [
86 "http://www.publisher.com/today/site?param1=a¶m2=b"
87 ],
88 "User-Agent": [
89 "Mozilla/5.0 (Linux; Android 8.0.0; SM-G960F Build/R16NW) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.84 Mobile Safari/537.36"
90 ]
91 },
92 "body": {
adapters/jixie/jixietest/exemplary/banner-and-video-site.json
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.
decided not to add the lang and dnt to the headers after all. Changed jixie.go 0xaa2d1d5
adapters/jixie/jixie.go
Outdated
|
||
//func buildEndpoint(endpoint string, timeout int64) string { | ||
// return endpoint + "?pstimeout=" + strconv.FormatInt(timeout, 10) | ||
//} |
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 commented lines
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.
removed . tks ( 0xaa2d1d5)
adapters/jixie/jixie.go
Outdated
addHeaderIfNonEmpty(headers, "Referer", request.Site.Page) | ||
} | ||
|
||
//theurl := buildEndpoint(a.endpoint, request.TMax) |
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 commented lines
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. tks in 0xaa2d1d5
if response.StatusCode == http.StatusNoContent { | ||
// no bid response | ||
return nil, 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.
Other adapters return a BadInput
error when a bad request is received. Would it be useful to add that scenario in our context?
124 func (a *JixieAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
125
126 if response.StatusCode == http.StatusNoContent {
127 // no bid response
128 return nil, nil
129 }
+
+ if response.StatusCode == http.StatusBadRequest {
+ return nil, []error{&errortypes.BadInput{
+ Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
+ }}
+ }
130
131 if response.StatusCode != http.StatusOK {
132 return nil, []error{&errortypes.BadServerResponse{
133 Message: fmt.Sprintf("Invalid Status Returned: %d. Run with request.debug = 1 for more info", response.StatusCode),
134 }}
135 }
adapters/jixie/jixie.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.
added. tks. in 0xaa2d1d5
-more checking in makerequest of the bidder params (added 2 more test jsons) -removed blank lines, lines commented out -test_params: a case with unit alone -BadInput error
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.
Thank you for addressing our feedback. Your adapter looks pretty good I just have a couple more questions.
adapters/jixie/jixie.go
Outdated
} | ||
} | ||
|
||
func processImp(imp *openrtb.Imp) error { |
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.
Given that Prebid Server core does adapter parameter validation on all Imp[i].ext
elements, we can probably spare the processImp(imp *openrtb.Imp)
function altoghether because checking for unmarshalling errors and the existence of a non-empty unit
field seems to be its sole purpose.
We can accomplish this by adding a minLength
restriction to the unit
property in file static/bidder-params/jixie.json
:
1 {
2 "$schema": "http://json-schema.org/draft-04/schema#",
3 "title": "Jixie Adapter Params",
4 "description": "A schema which validates params accepted by the Jixie adapter",
5 "type": "object",
6 "properties": {
7 "unit" : {
8 "type": "string",
9 "description": "The unit code of an inventory target",
+ "minLength": 1
10 },
11 "accountid" : {
static/bidder-params/jixie.json
The following test cases verify this point:
41 var invalidParams = []string{
42 `null`,
43 `nil`,
44 ``,
45 `[]`,
46 `true`,
47 `{}`,
48 `{"unit":12345678}`,
49 `{"Unit":"12345678"}`,
50 `{"unit":"1000008-AA77BB88CC", "bidfloor": 0.01}`,
51 `{"Unit": 12345678}`,
52 `{"AdUnit": "1"}`,
53 `{"adUnit": 1}`,
+ // empty required argument
+ `{"unit": ""}`,
+ // malformed, missing accountid value
+ `{"unit":"1000008-AA77BB88CC", "accountid", "bidfloor": "0.01", "jxprop1": "somethingimportant" }`,
+ // malformed
+ `{"unit":"1000008-AA77BB88CC", malformed, }`,
54 }
adapters/jixie/params_test.go
If implemented, we wouldn't need jixietest/supplemental/all-imps-missing-unit.json
anymore.
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 minLen 18 to be our criteria for unit. didn't know there is this minLength Thanks!!
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 minLen 18 to be our criteria for unit. didn't know there is this minLength Thanks!!
@jxdeveloper1 there's even a "pattern"
restriction that validates the value of a field matches a regular expression. Do you think it'd be useful in our context?
For instance, if we wanted unit
to always look like "1000008-AA77BB88CC"
, we could probably:
6 "properties": {
7 "unit" : {
8 "type": "string",
9 "description": "The unit code of an inventory target",
10 - "minLength": 18
+ "pattern": "^[0-9]{7}-[0-9A-Z]{10}$"
11 },
static/bidder-params/jixie.json
But if you want to use "minLenght"
I'm ok either way.
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.
Consider removing processImp
, with recent changes it seems to not be called anywhere anymore.
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.
Good catch. If this method exists just for verification, we can remove it and rely on built in functionality of PBS-Core.
} | ||
|
||
func (a *JixieAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
var errs = make([]error, 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.
If we discard the processImp(imp *openrtb.Imp)
function we could go for a leaner MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo)
version that only marshals and sets headers:
57 func (a *JixieAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
58 var errs = make([]error, 0)
59 -
60 - // copy the request, because we are going to mutate it
61 - requestCopy := *request
62 - // this will contain all the valid impressions
63 - var validImps []openrtb.Imp
64 - // pre-process the imps
65 - for _, imp := range requestCopy.Imp {
66 - if err := processImp(&imp); err == nil {
67 - validImps = append(validImps, imp)
68 - } else {
69 - errs = append(errs, err)
70 - }
71 - }
72 - if len(validImps) == 0 {
73 - err := &errortypes.BadInput{
74 - Message: "No valid impressions for jixie",
75 - }
76 - errs = append(errs, err)
77 - return nil, errs
78 - }
79 - requestCopy.Imp = validImps
80
81 - data, err := json.Marshal(requestCopy)
+ data, err := json.Marshal(request)
82 if err != nil {
83 errs = append(errs, err)
84 return nil, errs
85 }
86
87 headers := http.Header{}
88 headers.Add("Content-Type", "application/json;charset=utf-8")
89 headers.Add("Accept", "application/json")
90 if request.Device != nil {
91 addHeaderIfNonEmpty(headers, "User-Agent", request.Device.UA)
92 addHeaderIfNonEmpty(headers, "X-Forwarded-For", request.Device.IP)
93 }
94
95 if request.Site != nil {
96 addHeaderIfNonEmpty(headers, "Referer", request.Site.Page)
97 }
98
99 return []*adapters.RequestData{{
100 Method: "POST",
101 Uri: a.endpoint,
102 Body: data,
103 Headers: headers,
104 }}, errs
105 }
adapters/jixie/jixie.go
If implemented, we wouldn't need jixietest/supplemental/all-imps-missing-unit.json
anymore.
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.
cleaned up ; tks
adapters/jixie/jixie.go
Outdated
}}, errs | ||
} | ||
|
||
func ContainsAny(raw string, keys []string) bool { |
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's probably no need to export this function. Consider renaming to lowercase
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.
-> containsAny
…r-params/jixie.json file. removed checking in jixie.go that has become unnecssary. removed unnec test cases. updated params-test
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.
Your adapter looks pretty good to me. Left a couple more comments for your consideration.
Also, we usually encourage a test coverage of over a 90% for new adapters.
╰─➤ go test -coverprofile=c.out
PASS
coverage: 70.2% of statements
ok github.com/prebid/prebid-server/adapters/jixie 0.467s
╰─➤ go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/jixie/jixie.go:21: Builder 100.0%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:29: addHeaderIfNonEmpty 100.0%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:35: processImp 0.0%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:57: MakeRequests 85.7%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:86: containsAny 100.0%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:97: getBidType 100.0%
github.com/prebid/prebid-server/adapters/jixie/jixie.go:105: MakeBids 75.0%
github.com/prebid/prebid-server/adapters/jixie/usersync.go:10: NewJixieSyncer 100.0%
total: (statements) 70.2%
Thank you for addressing our feedback
adapters/jixie/jixie.go
Outdated
} | ||
} | ||
|
||
func processImp(imp *openrtb.Imp) error { |
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.
Consider removing processImp
, with recent changes it seems to not be called anywhere anymore.
adapters/jixie/jixie.go
Outdated
"github.com/prebid/prebid-server/openrtb_ext" | ||
) | ||
|
||
type JixieAdapter 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 name this adapter
. There is no need to export it with a upper case letter. There is no need to repeat the package name.
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 in commit 0x3afae48. thank you
adapters/jixie/jixie.go
Outdated
} | ||
} | ||
|
||
func processImp(imp *openrtb.Imp) error { |
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.
Good catch. If this method exists just for verification, we can remove it and rely on built in functionality of PBS-Core.
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 pretty good to me. Thank you very much for addressing the feedback.
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.
@jxdeveloper1 A couple of things before approving:
- I'm sorry I forgot to mention this in my review, but we encourage adapters to include the bid floor in the built-in
Imp[i].BidFloor
field instead of
defining a bidFloor param altogether? In other words:
{
"id": "some_test_auction",
"imp": [{
"id": "some_test_ad_id_1",
"banner": {
"w": 300,
"h": 100
},
+ "bidfloor": "0.01"
"ext": {
"bidder": {
"unit": "1000008-AbCdEf1234",
- "bidfloor": "0.01"
}
}
}],
"device": {
"ua": "Mozilla/5.0 (Linux; Android 8.0.0; SM-G960F Build/R16NW) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.84 Mobile Safari/537.36",
"ip": "111.222.333.444"
},
"site": {
"domain": "www.publisher.com",
"page": "http://www.publisher.com/today/site?param1=a¶m2=b"
}
}
If built-in Imp[i].BidFloor
field is used, we no longer need to add the bidfloor
param in the static/bidder-params/jixie.json
file
1 {
2 "$schema": "http://json-schema.org/draft-04/schema#",
3 "title": "Jixie Adapter Params",
4 "description": "A schema which validates params accepted by the Jixie adapter",
5 "type": "object",
6 "properties": {
7 "unit" : {
8 "type": "string",
9 "description": "The unit code of an inventory target",
10 "minLength": 18
11 },
.
.
20 "jxprop2" : {
21 "type": "string",
22 "description": "jxprop2 of an inventory target"
23 },
24 - "bidfloor": {
25 - "type": "string",
26 - "description": "The minimum price acceptable for a bid"
27 - }
28 },
29 "required": ["unit"]
30 }
static/bidder-params/jixie.json
Nor the BidFloor
field in the openrtb_ext/imp_jixie.go
file:
1 package openrtb_ext
2
3 type ExtImpJixie struct {
4 Unit string `json:"unit"`
5 AccountId string `json:"accountid,omitempty"`
6 JxProp1 string `json:"jxprop1,omitempty"`
7 JxProp2 string `json:"jxprop2,omitempty"`
8 - BidFloor string `json:"bidfloor,omitempty"`
9 }
openrtb_ext/imp_jixie.go
- Can you please update your documents with the complete list of params?
https://github.com/prebid/prebid.github.io/blob/d72deb6eae00b130e8fdb8329283fcf7e27bfce6/dev-docs/bidders/jixie.md
I'm sorry to bug you again. Other than these two, your adapter looks pretty good.
Hi great thanks once again: @guscarreon I have a big favour to ask - I have 3 questions. Do you think you can help me with them or tell me what could be a suitable place for me to raise such questions? QUESTION 1: i.e. This publisher only need to ensure their stored-requests (or whatever) submitted includes “jixie” (jixie “unit” ..). (i.e. They do not need to “request” Xandr to update so that it has the Jixie adapter?) QUESTION 2: QUESTION 3: (Sorry ... this one could be a bit crazy/ignorant; trying my luck) --But e.g. monitoring the Prometheus metrics won't there be a problem?(Works fine if I run prebid-server locally (nondocker) Maybe I am thinking too simplistically. I’d imagine people do put prebid-server in the cloud in this kind of setup. Just wondering what they do then. |
Some answers: QUESTION 2: QUESTION 3: |
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. @jxdeveloper1 thank you for addressing our feedback and for doing it so fast. My teammate @hhhjort provided answers for you but if you have any further questions, please let us know.
I also approved your documents PR. Next, a second reviewer will take a look at both pull requests.
thanks @hhhjort for your answers! (I removed my silly question) |
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
No description provided.