Adding Events support in bid responses #1597

laurb9 · 2020-11-24T04:14:57Z

Implemented wurl insertion in cached bids, vast rewriting and bid.ext.prebid.events for #1015
Updated and refactored auction_test unit tests for cached bids
Updated exchange_test for bid.ext.prebid.events and vast rewriting

Test coverage

ok github.com/prebid/prebid-server/exchange 6.039s coverage: 90.5% of statements
ok github.com/prebid/prebid-server/endpoints/events 0.022s coverage: 97.4% of statements

PG is deferred to later PR (PBS-Java does not implement it either)

CC @bretg

…id.events Missing timestamp (waiting for prebid#1584)

- wurl insertion in cached bids - vast modifying in video bids

This avoids unnecessary conversions between json and string ok github.com/prebid/prebid-server/endpoints/events 0.034s coverage: 96.1% of statements

laurb9 · 2020-12-01T21:41:37Z

Rebased after #1532

SyntaxNode

I didn't review the tests in detail yet. Have some other comments first.

endpoints/events/vtrack.go

exchange/events.go

SyntaxNode · 2020-12-08T07:28:32Z

openrtb_ext/request.go

@@ -23,6 +23,7 @@ type ExtRequestPrebid struct {
 	SChains              []*ExtRequestPrebidSChain `json:"schains,omitempty"`
 	StoredRequest        *ExtStoredRequest         `json:"storedrequest,omitempty"`
 	Targeting            *ExtRequestTargeting      `json:"targeting,omitempty"`
+	Events               json.RawMessage           `json:"events,omitempty"`


Can this be a strongly typed struct instead?

It can, but it would be an empty struct, as the only possible value so far is events: {}.

Ah. Is this like caching where the presence is the trigger?

ext.prebid.cache needs a value though (vastxml and/or bids).
I expect that in time, so will ext.prebid.events, but for now it works as you said.

SyntaxNode · 2020-12-08T07:29:43Z

exchange/exchange.go

@@ -52,11 +52,13 @@ type IdFetcher interface {

 type exchange struct {
 	adapterMap          map[openrtb_ext.BidderName]adaptedBidder
+	bidderInfo          adapters.BidderInfos


Do we really need the whole info, or perhaps just a boolean lookup for if vast is enabled for the bidders?

I thought to keep it generic, so if we add per-channel settings later, or need to look up other bidder settings, we would not need to go back and modify the exchange struct and all the unit tests.

Performance-wise, it looks equivalent, only one map lookup is done either way to reach the value.
Let me know your thoughts.

I wasn't worried about performance. Thought it might be simpler to only pass the information needed. I feel 50/50 on it. I imagine we'll refactor the bidderInfo map to some kind of bidderInfoService in the future anyway. I think we might need more context after I add in hardcoded aliases.

- Fixed comments - Removed redundant types in func proto - Add bool parameter to `ModifyVastXmlString()` to indicate string was changed - Decode RawMessage instead of casting to string (to handle encoded escapes) > ok github.com/prebid/prebid-server/endpoints/events 0.039s coverage: 97.4% of statements

When the VAST is modified to insert the event tracking tag, the modified creative is also returned in the bid response, in addition to being cached. Behavior currently implemented requires targeting enabled. Also renamed eventsData->eventTracking

laurb9 · 2020-12-10T03:14:45Z

I implemented part of the spec change mentioned here #1015 (comment) - the modified VAST creative should be returned in bid response, not only cache.

I moved the VAST rewriting from auction/doCache to exchange/HoldAuction with the associated unit test migration, as they use two different sets of json specs. There are still some unanswered questions regarding the requirement that targeting be enabled for VAST rewriting because only the winning bids are modified, or even if we should modify all the bids instead.

For now, this implementation requires targeting, until the requirements are clarified. This should be ok if most requests use GAM or some other network that needs the targeting keys anyway, so targeting would be enabled.

Also an open question regarding the PBS behavior of making up a VAST creative from the nurl if it was missing, but only to cache it, while still returning an empty adm. Combining this behavior, if intentional, with the requirement to always return modified VAST is going to be difficult. For this PR, PBS will return the modified made-up creative if it needed to be modified, and fall back to the original behavior otherwise.

- all bids have vast modified if account and bidder enabled, not just winning - all NON-VIDEO bids have events if account or request enabled - all cached NON-VIDEO bids have wurl patched in if account or request enabled. prebid#1015 (comment)

bsardo

LGTM

laurb9 marked this pull request as draft November 24, 2020 17:38

laurb9 changed the title ~~Adding Events (WIP) PBID-414, PBID-413, PBID-958~~ Adding Events (WIP) #1015 Nov 24, 2020

laurb9 changed the title ~~Adding Events (WIP) #1015~~ Adding Events support in bid responses (WIP) #1015 Nov 24, 2020

laurb9 mentioned this pull request Nov 24, 2020

PBS video impression tracking #1015

Closed

laurb9 force-pushed the events branch 3 times, most recently from 04f7d0c to 1849877 Compare November 25, 2020 19:03

laurb9 force-pushed the events branch 2 times, most recently from cfc30bc to b829081 Compare December 1, 2020 21:26

laurb9 added 4 commits December 1, 2020 21:31

Initial implementation - cache wurl, cache vast rewrite, bid.ext.preb…

57bb6f1

…id.events Missing timestamp (waiting for prebid#1584)

Cache unit tests, some test refactoring.

5e3b8cc

- wurl insertion in cached bids - vast modifying in video bids

Add timestamp to event after prebid#1584 was merged

4fac314

Extracted vtrack.ModifyVastXmlString to avoid conversions

3298144

This avoids unnecessary conversions between json and string ok github.com/prebid/prebid-server/endpoints/events 0.034s coverage: 96.1% of statements

laurb9 force-pushed the events branch from b829081 to 3298144 Compare December 1, 2020 21:31

laurb9 marked this pull request as ready for review December 1, 2020 21:39

SyntaxNode requested review from bsardo and mansinahar December 3, 2020 15:14

SyntaxNode assigned bsardo and mansinahar Dec 3, 2020

laurb9 changed the title ~~Adding Events support in bid responses (WIP) #1015~~ Adding Events support in bid responses #1015 Dec 4, 2020

laurb9 changed the title ~~Adding Events support in bid responses #1015~~ Adding Events support in bid responses Dec 8, 2020

SyntaxNode reviewed Dec 8, 2020

View reviewed changes

laurb9 force-pushed the events branch from c85e0ad to 4418a39 Compare December 8, 2020 22:46

laurb9 force-pushed the events branch from 4418a39 to 420bcfb Compare December 8, 2020 23:01

laurb9 added 2 commits December 15, 2020 04:39

Merge branch 'master' into events

1613af9

Modify unit tests to updated spec:

6797b3a

- all bids have vast modified if account and bidder enabled, not just winning - all NON-VIDEO bids have events if account or request enabled - all cached NON-VIDEO bids have wurl patched in if account or request enabled. prebid#1015 (comment)

bsardo approved these changes Jan 20, 2021

View reviewed changes

bsardo merged commit 994d0f0 into prebid:master Jan 20, 2021

laurb9 deleted the events branch February 25, 2021 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding Events support in bid responses #1597

Adding Events support in bid responses #1597

Uh oh!

laurb9 commented Nov 24, 2020 •

edited

Loading

Uh oh!

laurb9 commented Dec 1, 2020

Uh oh!

SyntaxNode left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SyntaxNode Dec 8, 2020

Uh oh!

laurb9 Dec 8, 2020

Uh oh!

SyntaxNode Dec 8, 2020

Uh oh!

laurb9 Dec 8, 2020

Uh oh!

SyntaxNode Dec 8, 2020

Uh oh!

laurb9 Dec 8, 2020

Uh oh!

SyntaxNode Dec 8, 2020

Uh oh!

laurb9 commented Dec 10, 2020 •

edited

Loading

Uh oh!

bsardo left a comment

Uh oh!

Uh oh!

Adding Events support in bid responses #1597

Adding Events support in bid responses #1597

Uh oh!

Conversation

laurb9 commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurb9 commented Dec 1, 2020

Uh oh!

SyntaxNode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SyntaxNode Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

laurb9 Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

SyntaxNode Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

laurb9 Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

SyntaxNode Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

laurb9 Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

SyntaxNode Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

laurb9 commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsardo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laurb9 commented Nov 24, 2020 •

edited

Loading

laurb9 commented Dec 10, 2020 •

edited

Loading