Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improving /setuid #86

Closed
wants to merge 35 commits into from
Closed

Improving /setuid #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
70e0460
Added interfaces for metrics classes.
dbemiller Jul 17, 2017
c63e72a
First pass at a working taggable metrics module.
dbemiller Jul 17, 2017
97795b3
Better API for the influx metric module.
dbemiller Jul 18, 2017
f599a67
Implemented the Influx metrics using Tags for the server.
dbemiller Jul 18, 2017
4a09b3b
Added cookie sync counter, which we would also want to replace.
dbemiller Jul 18, 2017
a08c16d
Added some unit tests, and fixed a bug.
dbemiller Jul 18, 2017
3ae9ecd
Added unit tests for the reporter, and fixed some bugs.
dbemiller Jul 20, 2017
b50bcb6
Made everything private which could be made private. Added more tests.
dbemiller Jul 20, 2017
81d0b8d
Added more thorough unit tests, and fixed some bugs.
dbemiller Jul 20, 2017
c84b0ba
Added a unit test for the cookiesync event logging.
dbemiller Jul 20, 2017
73d183f
Merge branch 'master' of https://github.com/prebid/prebid-server into…
dbemiller Jul 20, 2017
03d9373
Merge branch 'gometrics-metrics' of https://github.com/prebid/prebid-…
dbemiller Jul 20, 2017
4472519
Added tagged metrics throughout pbs_light. Removed the metrics_test c…
dbemiller Jul 21, 2017
bde0102
Removed some superfluous code, and fixed some documentation.
dbemiller Jul 21, 2017
e4d1dc9
Fixed style.
dbemiller Jul 21, 2017
5df9ec0
Removed pbs as a dependency from the metrics package. This will preve…
dbemiller Jul 28, 2017
deefaf5
Added metrics to capture the number of user sync requests per bidder.
dbemiller Jul 28, 2017
76bcc1b
Small refactor to bury the cookie behind an interface, to control mut…
dbemiller Aug 4, 2017
1eed01c
Replaced the raw userIDs map with the cookie, on the pbsrequest.
dbemiller Aug 4, 2017
1ab7763
Prevented PBS from saving 0 for audienceNetwork.
dbemiller Aug 7, 2017
bdf3585
Renamed some methods, and added a bunch of documentation.
dbemiller Aug 7, 2017
e057ca1
Small refactoring. Many added unit tests.
dbemiller Aug 7, 2017
3850b27
Added a test for request reading & writing.
dbemiller Aug 7, 2017
854c3b1
Reorganized the imports, to pass gofmt.
dbemiller Aug 7, 2017
ba81deb
Renamed IsAllowed, because it didnt make much sense.
dbemiller Aug 7, 2017
799f5ef
Polishing the code after seeing the diff.
dbemiller Aug 7, 2017
9fad2fc
Removed the nils from prod code. Added more tests.
dbemiller Aug 7, 2017
2d9a4b9
More thorough tests.
dbemiller Aug 7, 2017
9abf5ae
Fixed a potential bug with nil-panics on app requests. Renamed interf…
dbemiller Aug 7, 2017
3e4c71a
Fixed user sync metrics so that we dont count facebook uid=0.
dbemiller Aug 7, 2017
876b57d
Made the user sync metrics more granular, to separate out facebook er…
dbemiller Aug 7, 2017
4adbfaf
Merged from master, and fixed the conflicts
dbemiller Aug 8, 2017
705de9a
Merged from master. Tests/code may not validate anymore.
dbemiller Aug 9, 2017
91bb9f7
Used go-metrics to log usersyncs per adapter.
dbemiller Aug 9, 2017
bf21bfe
Merged from master. Fixed the conflict.
dbemiller Aug 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions adapters/appnexus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,10 @@ func TestAppNexusBasicResponse(t *testing.T) {
req.Header.Add("User-Agent", andata.deviceUA)
req.Header.Add("X-Real-IP", andata.deviceIP)

pc := pbs.ParseUIDCookie(req)
pc.UIDs["adnxs"] = andata.buyerUID
pc := pbs.ParseUserSyncMapFromRequest(req)
pc.TrySync("adnxs", andata.buyerUID)
fakewriter := httptest.NewRecorder()
pbs.SetUIDCookie(fakewriter, pc)
pc.SetCookieOnResponse(fakewriter, "")
req.Header.Add("Cookie", fakewriter.Header().Get("Set-Cookie"))

cacheClient, _ := dummycache.New()
Expand Down
6 changes: 3 additions & 3 deletions adapters/facebook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,10 @@ func TestFacebookBasicResponse(t *testing.T) {
req.Header.Add("User-Agent", fbdata.deviceUA)
req.Header.Add("X-Real-IP", fbdata.deviceIP)

pc := pbs.ParseUIDCookie(req)
pc.UIDs["audienceNetwork"] = fbdata.buyerUID
pc := pbs.ParseUserSyncMapFromRequest(req)
pc.TrySync("audienceNetwork", fbdata.buyerUID)
fakewriter := httptest.NewRecorder()
pbs.SetUIDCookie(fakewriter, pc)
pc.SetCookieOnResponse(fakewriter, "")
req.Header.Add("Cookie", fakewriter.Header().Get("Set-Cookie"))

cacheClient, _ := dummycache.New()
Expand Down
6 changes: 3 additions & 3 deletions adapters/pulsepoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,10 @@ func SampleRequest(numberOfImpressions int, t *testing.T) *pbs.PBSRequest {
// setup a http request
httpReq := httptest.NewRequest("POST", CreateService(BidOnTags("")).Server.URL, body)
httpReq.Header.Add("Referer", "http://news.pub/topnews")
pc := pbs.ParseUIDCookie(httpReq)
pc.UIDs["pulsepoint"] = "pulsepointUser123"
pc := pbs.ParseUserSyncMapFromRequest(httpReq)
pc.TrySync("pulsepoint", "pulsepointUser123")
fakewriter := httptest.NewRecorder()
pbs.SetUIDCookie(fakewriter, pc)
pc.SetCookieOnResponse(fakewriter, "")
httpReq.Header.Add("Cookie", fakewriter.Header().Get("Set-Cookie"))
// parse the http request
cacheClient, _ := dummycache.New()
Expand Down
6 changes: 3 additions & 3 deletions adapters/rubicon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,10 +259,10 @@ func TestRubiconBasicResponse(t *testing.T) {
req.Header.Add("User-Agent", rubidata.deviceUA)
req.Header.Add("X-Real-IP", rubidata.deviceIP)

pc := pbs.ParseUIDCookie(req)
pc.UIDs["rubicon"] = rubidata.buyerUID
pc := pbs.ParseUserSyncMapFromRequest(req)
pc.TrySync("rubicon", rubidata.buyerUID)
fakewriter := httptest.NewRecorder()
pbs.SetUIDCookie(fakewriter, pc)
pc.SetCookieOnResponse(fakewriter, "")
req.Header.Add("Cookie", fakewriter.Header().Get("Set-Cookie"))

cacheClient, _ := dummycache.New()
Expand Down
21 changes: 12 additions & 9 deletions pbs/pbsrequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ type PBSRequest struct {
Device *openrtb.Device `json:"device"`

// internal
Bidders []*PBSBidder `json:"-"`
UserIDs map[string]string `json:"-"`
Url string `json:"-"`
Domain string `json:"-"`
Bidders []*PBSBidder `json:"-"`
SyncMap UserSyncMap `json:"-"`
Url string `json:"-"`
Domain string `json:"-"`
Start time.Time
}

Expand Down Expand Up @@ -129,12 +129,12 @@ func ParsePBSRequest(r *http.Request, cache cache.Cache) (*PBSRequest, error) {

// use client-side data for web requests
if pbsReq.App == nil {
pc := ParseUIDCookie(r)
pbsReq.UserIDs = pc.UIDs
pc := ParseUserSyncMapFromRequest(r)
pbsReq.SyncMap = pc

// this would be for the shared adnxs.com domain
if anid, err := r.Cookie("uuid2"); err == nil {
pbsReq.UserIDs["adnxs"] = anid.Value
pbsReq.SyncMap.TrySync("adnxs", anid.Value)
}

pbsReq.Device.UA = r.Header.Get("User-Agent")
Expand Down Expand Up @@ -162,6 +162,8 @@ func ParsePBSRequest(r *http.Request, cache cache.Cache) (*PBSRequest, error) {
if err != nil {
return nil, fmt.Errorf("Invalid URL '%s': %v", url.Host, err)
}
} else {
pbsReq.SyncMap = NewSyncMap()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an empty SyncMap on mobile app requests for everything to work? Cookie sync doesn’t make sense on mobile app, so having an empty sync map could be confusing.

Copy link
Contributor Author

@dbemiller dbemiller Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is... I'm not sure. I definitely agree that it's confusing.

On the other hand... func (req *PBSRequest) GetUserID(BidderCode string) surfaced some interesting unit test failures. Apparently in Go any reads on a nil map are treated as if that map were empty. So pbsReq.UIDs['key'] will just return nil if UIDs is undefined, but req.SyncMap.GetUID() will cause panic. Stashing an empty SyncMap on here makes reads behave nicely.

I don't know offhand whether any of the other PBS code attempts to read cookiesyncs inside the "mobile" code branches... but as a general rule, ab object should be equally usable regardless of how its constructed.

I think this lends some extra weight to your suggestion here. If the methods are defined on a struct, then I can define how they behave on a nil. Interfaces don't give that flexibility.

}

if r.FormValue("debug") == "1" {
Expand Down Expand Up @@ -229,8 +231,9 @@ func (req PBSRequest) Elapsed() int {
return int(time.Since(req.Start) / 1000000)
}

func (req PBSRequest) GetUserID(BidderCode string) string {
if uid, ok := req.UserIDs[BidderCode]; ok {
func (req *PBSRequest) GetUserID(BidderCode string) string {
if req.SyncMap != nil {
uid, _ := req.SyncMap.GetUID(BidderCode)
return uid
}
return ""
Expand Down
Loading