Separate Usersyncs from Bidders #310

dbemiller · 2018-01-31T15:28:00Z

This fixes #300.

The goal here is so that new bidders can specify their user sync endpoints OpenRTB-only submissions do user syncs without having to rely on the legacy adapter interface.

Today we have legacy and OpenRTB 2 bidders. I'm sure in the future we'll have OpenRTB 3 bidders.... so it seemed more maintainable to me to separate usersyncs into their own interface.

…endpoint into the endpoints package.

…r map rather than the legacy adapters.

…e source of truth.

…a bit easier to find.

… usersync-refactor

hhhjort · 2018-02-05T19:46:17Z

pbs_light.go

+type cookieSyncDeps struct {
+	syncers      map[openrtb_ext.BidderName]usersyncers.Usersyncer
+	optOutCookie *config.Cookie
+	metric       metrics.Meter


I was going to suggest that metric be a struct so additional metrics could be added in later if needed, but on second thought we can probably change that easily enough if and when more metrics are desired.

ok... let me know if you change your mind. I don't have strong feelings either way... but I kept it simple because we don't have any immediate need for more metrics here, and I suspect that #302 will get merged and start to absorb the metrics code before we do.

hhhjort

Better way to organize. A lot of little changes to yank it out of all the bidders, but I haven't caught any issues yet.

cindyhere33 · 2018-02-06T17:25:38Z

pbs_light.go

@@ -313,10 +321,13 @@ func (deps *auctionDeps) auction(w http.ResponseWriter, r *http.Request, _ httpr
 			ametrics.RequestMeter.Mark(1)
 			accountAdapterMetric.RequestMeter.Mark(1)
 			if pbs_req.App == nil {
-				uid, _, _ := pbs_req.Cookie.GetUID(ex.FamilyName())
+				// If exchanges[bidderCode] exists, then deps.syncers[bidderCode] should exist too.
+				// The unit tests guarantee it.


Can you help me identify the unit test?

exchanges is a map of its own and NewSyncerMap() creates a map again. This works best when the two of them don't interact with each other. I only wonder if this is always true:

If exchanges[bidderCode] exists, then deps.syncers[bidderCode] should exist too.

After looking into it... you're totally right. This comment is lying, and there's a bug here.

districtm is part of the exchange, but not the NewSyncerMap(). TestSyncers inside usersync/usersync_test.go makes sure that the NewSyncerMap() is maintained with openrtb_ext.BidderMap, but there's no analogous test for the exchange.

I'll find a way to hack the legacy code to work with districtm, and add a new test to make sure that all the bidders except districtm are in the usersyncer map.

All fixed ^^. It's very hacky, but... at least the hacks are limited to the legacy code.

cindyhere33 · 2018-02-06T17:27:20Z

pbs_light.go

@@ -838,12 +848,14 @@ func serve(cfg *config.Configuration) error {
 		glog.Fatalf("Failed to create the amp endpoint handler. %v", err)
 	}

+	syncers := usersyncers.NewSyncerMap(cfg)


The main BidderMap is in openrtb2_ext package. I know exchanges is from old code but wouldn't it be better if we could have a single source for all bidders?

Definitely. I'm guilty of ignoring legacy code, but... this isn't much effort, and you're right that it's valuable.

Actually... there is a problem with this. The exchange includes districtm, which is an appnexus alias. Alias support is added to openrtb_ext in #296... but it's done dynamically, because this style of hardcoded alias won't be able to support the dynamic pbjs.addBidderAlias() calls in JS.

So... there actually isn't a single source of truth here. The BidderMap is a source of truth for Usersyncer and Bidder impls... but the exchange doesn't fit into that.

I really did like the idea, though.

Yes, single source excluding exchanges. Perfect! 😄

yeah... in that case, there is. The TestSyncers test inside usersync/usersync_test.go makes sure that the Usersyncer map stays up to date with the BidderMap, and the TestNewAdapterMap test inside exchange/adapter_map_test.go makes sure that the OpenRTB Exchange stays in sync with it.

Nothing is watching out for the legacy exchange, though.

True the test accomplishes it but I believe the code should be made fail-proof regardless of a test (at least we should try to as much as we can). It would need a switch-case in the NewSyncerMap() that iterates through the BidderMap.

I'll leave it to you to decide if it's worth it. :)

Don't think so... that just replaces a map declaration with a switch statement. We'd still have to update two places in the code whenever we add a new Bidder, so it's still definitely a point of failure (barring the unit tests)

cindyhere33

Good move!

* Added a bunch of usersync code to the module, and moved the usersync endpoint into the endpoints package. * Added an index adapter, and made pbs_light use the map. * Fixed a bug and added some tests. Made the auction code use the syncer map rather than the legacy adapters. * Deleted lots of old usersync code in the legacy adapters. * Moved syncers out of the global state. * Made the BidderNames consistent, and made the BidderName list a single source of truth. * Better unit test. * Renamed endpoints/usersync to endpoints/cookie_sync, which should be a bit easier to find. * Removed unnecessary test. * Undid some code moves which didnt need to be part of this PR. * Updated documentation. * Trimmed a sentence I dont think is helpful. * Updated tests to fix CI. * Fixed a bug, added some tests, and updated a comment with more details.

* OTT-583 :: Refactored floor code and used RequestWrapper for retrieving and setting extension object (prebid#310) * OTT-589: Eids is getting passed randomly to user.eids for appnexus adapter (prebid#311) Co-authored-by: Nikhil Vaidya<nikhi.vaidya@pubmatic.com> Co-authored-by: Pubmatic-Dhruv-Sonone <83747371+Pubmatic-Dhruv-Sonone@users.noreply.github.com>

* OTT-356: Setting WB=1 for adpod bids for new generate bid id feature (prebid#304) * OTT-478: Added updatedrequest in debug log to know about floor rule selection (prebid#306) * OTT-478: Fixed UT for floors enforcement (prebid#308) * OTT-583 :: Refactored floor code and used RequestWrapper for retrieving and setting extension object (prebid#310) * OTT-589: Eids is getting passed randomly to user.eids for appnexus adapter (prebid#311) * OTT-583: Added formating (prebid#318) * OTT-479: Updated for currency conversion related valiation and error in rejected bids (prebid#320) * OTT-598:: Added check if floor data is nil * OTT-596: Updated error message for currency conversion error (prebid#322) * OTT-596: Fixed UT's for error message for currency conversion error (prebid#323) Co-authored-by: Jaydeep Mohite <30924180+pm-jaydeep-mohite@users.noreply.github.com> Co-authored-by: Jaydeep Mohite <jaydeep.mohite@pubmatic.com> Co-authored-by: Viral Vala <63396712+pm-viral-vala@users.noreply.github.com> Co-authored-by: Pubmatic-Dhruv-Sonone <83747371+Pubmatic-Dhruv-Sonone@users.noreply.github.com> Co-authored-by: PubMatic-OpenWrap <UOEDev@pubmatic.com>

…nic-with-empty-banner-pbs PB-1192 fixed panic if empty format

* set up cache multiple ads exp * configure max cached ads count * update

dbemiller added 14 commits January 29, 2018 13:14

Added a bunch of usersync code to the module, and moved the usersync …

ddfd0f7

…endpoint into the endpoints package.

Added an index adapter, and made pbs_light use the map.

7a88d36

Merged from master. Fixed conflicts.

5633d4b

Fixed a bug and added some tests. Made the auction code use the synce…

c694a99

…r map rather than the legacy adapters.

Deleted lots of old usersync code in the legacy adapters.

06d972c

Moved syncers out of the global state.

45351d3

Made the BidderNames consistent, and made the BidderName list a singl…

d292636

…e source of truth.

Better unit test.

b535c05

Renamed endpoints/usersync to endpoints/cookie_sync, which should be …

f160eba

…a bit easier to find.

Merged from master. Fixed conflicts.

83528a3

Removed unnecessary test.

bd5636c

Undid some code moves which didnt need to be part of this PR.

6612ec5

Updated documentation.

ad5b468

Trimmed a sentence I dont think is helpful.

28d5feb

dbemiller assigned hhhjort and dbemiller and unassigned hhhjort Jan 31, 2018

dbemiller added 3 commits February 1, 2018 10:57

Merged from master. Fixed conflicts.

539aeef

Merge branch 'master' of https://github.com/prebid/prebid-server into…

24ec857

… usersync-refactor

Updated tests to fix CI.

fb85fc3

dbemiller requested a review from hhhjort February 5, 2018 17:39

dbemiller assigned ndhimehta and hhhjort and unassigned dbemiller Feb 5, 2018

ndhimehta assigned cindyhere33 Feb 5, 2018

dbemiller unassigned ndhimehta Feb 5, 2018

hhhjort reviewed Feb 5, 2018

View reviewed changes

hhhjort previously approved these changes Feb 5, 2018

View reviewed changes

cindyhere33 reviewed Feb 6, 2018

View reviewed changes

Fixed a bug, added some tests, and updated a comment with more details.

0070083

dbemiller dismissed hhhjort’s stale review via 0070083 February 6, 2018 18:07

cindyhere33 approved these changes Feb 6, 2018

View reviewed changes

dbemiller merged commit 8f199af into master Feb 6, 2018

dbemiller deleted the usersync-refactor branch February 6, 2018 20:09

allar15 added a commit to allar15/prebid-server that referenced this pull request Nov 24, 2023

Merge pull request prebid#310 from nextmillenniummedia/PB-1192-fix-pa…

98ee2a2

…nic-with-empty-banner-pbs PB-1192 fixed panic if empty format

StarWindMoonCloud pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jun 5, 2024

set up cache multiple ads exp (prebid#310)

7cfa187

* set up cache multiple ads exp * configure max cached ads count * update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Usersyncs from Bidders #310

Separate Usersyncs from Bidders #310

dbemiller commented Jan 31, 2018 •

edited

Loading

hhhjort Feb 5, 2018

dbemiller Feb 5, 2018

hhhjort left a comment

cindyhere33 Feb 6, 2018

dbemiller Feb 6, 2018 •

edited

Loading

dbemiller Feb 6, 2018

cindyhere33 Feb 6, 2018

dbemiller Feb 6, 2018

dbemiller Feb 6, 2018 •

edited

Loading

cindyhere33 Feb 6, 2018

dbemiller Feb 6, 2018 •

edited

Loading

cindyhere33 Feb 6, 2018

dbemiller Feb 6, 2018

cindyhere33 left a comment

Separate Usersyncs from Bidders #310

Separate Usersyncs from Bidders #310

Conversation

dbemiller commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyhere33 left a comment

Choose a reason for hiding this comment

dbemiller commented Jan 31, 2018 •

edited

Loading

dbemiller Feb 6, 2018 •

edited

Loading

dbemiller Feb 6, 2018 •

edited

Loading

dbemiller Feb 6, 2018 •

edited

Loading