OpenRTB Prebid Cache support #276

dbemiller · 2018-01-10T21:16:42Z

This fixes #216 and #199

The code is... pretty ugly, and a little embarrassing.

Normally I'd like to clean this up... but Prebid Mobile is being blocked by OpenRTB support in PBS, and this isn't the only thing left to do, so... I've gotta scope it somewhere.

…for consistency.

…he new client can call it.

…enRTB serialization into the exchange.

…k behavior was actually untestable.

dbemiller · 2018-01-10T21:18:02Z

@ndhimehta please send this to someone on rubicon as the second reviewer.

ndhimehta · 2018-01-10T21:40:47Z

@dbemiller Review scheduled for WO - 1/15.

hhhjort · 2018-01-16T16:18:03Z

exchange/auction.go

+	}
+}
+
+func (auction *auction) numImps() int {


Is this function supposed to have a purpose outside of the one test case?

It makes auction nil-safe. The idea was to keep the "do we need to track winning bids?" logic in one place. If not, the exchange can initialize the variable to a nil. If so, the exchange can initialize a real auction object.

Then the rest of the exchange logic can call these functions as if it needed to track the winning bids without much performance impact.

Wait I'm sorry.... no, you're right. It's not called anywhere real.

I'll delete it.

hhhjort · 2018-01-16T16:25:20Z

exchange/auction.go

+	}
+}
+
+func (auction *auction) forEachCachedBid(callback func(bid *openrtb.Bid, id string)) {


You don't seem to be using forEachCacheBid() at all?

Are you planning an interface definition that auction will need to implement, and if so shouldn't these be public functions?

No plans... just sloppy of me. I'll delete it.

hhhjort · 2018-01-16T16:33:24Z

exchange/cache.go

+	"strings"
+)
+
+// cacheBids stores the given Bids in prebid cache, and saves the generated UUIDs in the auction.


This caches all the best bids from each bidder from my reading. In any case you don't feed this function a list of bids to cache. Might want to clean up the comment, especially in light of the potential that we will eventually implement multiple different bid caching strategies, and this implements one specific strategy.

agreed, thanks. I updated the comments to match.

hhhjort

Looks good, and OpenRTB needs cache support.

… cache-bids-support

…blishers dont need to update their creatives in DFP.

dbemiller · 2018-01-16T21:58:41Z

One noteworthy change in the last few updates: @anwzhang pointed out that legacy mobile used hb_cache_id rather than hb_uuid. If we change this name, publishers have to update all their creatives in DFP.

So... I renamed the param to hb_cache_id, for legacy compatibility.

sandraleon

just had a couple questions but looks good overall.

sandraleon · 2018-01-16T22:17:20Z

exchange/auction.go

+
+func (auction *auction) cacheId(bid *openrtb.Bid) (id string, exists bool) {
+	id, exists = auction.cachedBids[bid]
+	return


this returns values for id and exists?

yes: https://tour.golang.org/basics/7

sandraleon · 2018-01-16T22:18:49Z

exchange/auction.go

+
+// forEachBestBid runs the callback function on every bid which is the highest one for each Bidder on each Imp.
+func (auction *auction) forEachBestBid(callback func(impID string, bidder openrtb_ext.BidderName, bid *openrtb.Bid, winner bool)) {
+	if auction == nil {


is it necessary to check this? can auction be nil if it was able to call this function?

no, you're right, it's not necessary. auction is non-nil if targeting is non-nil... and the only call to this function is after an if targeting == nil { return; } block.

I removed it.

sandraleon · 2018-01-16T22:35:39Z

exchange/exchange.go

 	e := new(exchange)

 	e.adapterMap = newAdapterMap(client, cfg)
 	e.adapters = make([]openrtb_ext.BidderName, 0, len(e.adapterMap))
+	e.cache = cache
+	e.cacheTime = time.Duration(cfg.CacheURL.ExpectedTimeMillis) * time.Millisecond


if this is not set in the config, cache time will be zero?

correct. Do you think that makes sense as a default, or would it be better to have some time here?

Per the note here, I hope this doesn't become a long-term solution in any case.

seems fine to me, just wanted to make sure that was what we wanted.

hhhjort

Changes since last review look good.

* Added the cache API contract classes, and renamed some existing ones for consistency. * Fixed some more bugs in the unmarshalling. * Made a Prebid Cache client which saved the entire bid as a JSON value. * Added a new test, and improved the cache implementation. * Ran gofmt. * Moved the GetBaseURL function onto the config.Cache struct, so that the new client can call it. * Separated the prebid cache client dependencies from OpenRTB. Moved OpenRTB serialization into the exchange. * Simplified cache implementation, because some of the graceful fallback behavior was actually untestable. * Added a cache client to the exchange. * basic default cache behavior. * Cached the best bid from each bidder in each imp. * Fixed tests and some bugs. * Added unit tests for the auction state. * Removed cache from the gitignore. * Fixed some bugs, and added some more unit tests. * Added error log for critical errors. * Some code logic simplifications. * Fixed another bug and added some more tests. * Made the reserved cache time configurable. * Ran gofmt. * Fixed issue prebid#199 in OpenRTB. * Removed a few unused methods. * Updated comment to be more accurate. * Renamed hb_uuid to hb_cache_id, for consistency with today so that publishers dont need to update their creatives in DFP. * Removed an unnecessary nil check.

…-impression-release-tag added release tag to imppression pixel

* rollout algo2 in-feed * update * update

dbemiller added 19 commits January 5, 2018 11:07

Added the cache API contract classes, and renamed some existing ones …

0adbe76

…for consistency.

Fixed some more bugs in the unmarshalling.

c5f02c0

Made a Prebid Cache client which saved the entire bid as a JSON value.

167624b

Added a new test, and improved the cache implementation.

ccdf9e7

Ran gofmt.

466b291

Moved the GetBaseURL function onto the config.Cache struct, so that t…

3ed291f

…he new client can call it.

Separated the prebid cache client dependencies from OpenRTB. Moved Op…

8c576d9

…enRTB serialization into the exchange.

Simplified cache implementation, because some of the graceful fallbac…

785dc47

…k behavior was actually untestable.

Added a cache client to the exchange.

6ac6765

basic default cache behavior.

dbfab9e

Cached the best bid from each bidder in each imp.

578c1e5

Fixed tests and some bugs.

80caedd

Added unit tests for the auction state.

795d706

Removed cache from the gitignore.

672cbf9

Fixed some bugs, and added some more unit tests.

f3584f5

Added error log for critical errors.

272b4c4

Some code logic simplifications.

28d73e4

Fixed another bug and added some more tests.

13ebd07

Made the reserved cache time configurable.

ae04b53

ghost assigned dbemiller Jan 10, 2018

ghost added the in progress label Jan 10, 2018

dbemiller requested a review from hhhjort January 10, 2018 21:16

dbemiller assigned hhhjort and ndhimehta Jan 10, 2018

Ran gofmt.

2095cfd

ndhimehta assigned sandraleon and unassigned ndhimehta Jan 10, 2018

ndhimehta added the needs review label Jan 10, 2018

Fixed issue #199 in OpenRTB.

910c56f

dbemiller removed their assignment Jan 10, 2018

hhhjort reviewed Jan 16, 2018

View reviewed changes

Removed a few unused methods.

b04779e

ghost assigned dbemiller Jan 16, 2018

hhhjort reviewed Jan 16, 2018

View reviewed changes

hhhjort previously approved these changes Jan 16, 2018

View reviewed changes

dbemiller added 3 commits January 16, 2018 16:42

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

d16ef60

… cache-bids-support

Updated comment to be more accurate.

0a0a446

Renamed hb_uuid to hb_cache_id, for consistency with today so that pu…

1bb3724

…blishers dont need to update their creatives in DFP.

dbemiller dismissed hhhjort’s stale review via 1bb3724 January 16, 2018 21:56

sandraleon previously approved these changes Jan 16, 2018

View reviewed changes

Removed an unnecessary nil check.

251efc7

dbemiller dismissed sandraleon’s stale review via 251efc7 January 17, 2018 13:54

hhhjort approved these changes Jan 17, 2018

View reviewed changes

dbemiller merged commit 21da662 into master Jan 17, 2018

dbemiller deleted the cache-bids-support branch January 17, 2018 16:40

ghost removed the in progress label Jan 17, 2018

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

Merge pull request prebid#276 from nextmillenniummedia/PB-870-bug/PBS…

7c24818

…-impression-release-tag added release tag to imppression pixel

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

rollout algo2 in-feed (prebid#276)

0e9d505

* rollout algo2 in-feed * update * update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenRTB Prebid Cache support #276

OpenRTB Prebid Cache support #276

dbemiller commented Jan 10, 2018 •

edited

Loading

dbemiller commented Jan 10, 2018

ndhimehta commented Jan 10, 2018

hhhjort Jan 16, 2018

dbemiller Jan 16, 2018

dbemiller Jan 16, 2018

hhhjort Jan 16, 2018

dbemiller Jan 16, 2018

hhhjort Jan 16, 2018

dbemiller Jan 16, 2018

hhhjort left a comment

dbemiller commented Jan 16, 2018

sandraleon left a comment

sandraleon Jan 16, 2018

dbemiller Jan 17, 2018

sandraleon Jan 16, 2018

dbemiller Jan 17, 2018

sandraleon Jan 16, 2018

dbemiller Jan 17, 2018

sandraleon Jan 17, 2018

hhhjort left a comment

OpenRTB Prebid Cache support #276

OpenRTB Prebid Cache support #276

Conversation

dbemiller commented Jan 10, 2018 • edited Loading

dbemiller commented Jan 10, 2018

ndhimehta commented Jan 10, 2018

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

Choose a reason for hiding this comment

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

dbemiller commented Jan 16, 2018

sandraleon left a comment

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

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

hhhjort left a comment

Choose a reason for hiding this comment

dbemiller commented Jan 10, 2018 •

edited

Loading