-
Notifications
You must be signed in to change notification settings - Fork 742
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
PBS video impression tracking #1015
Comments
For load balancer reasons, I'm starting to prefer a separate endpoint /vtrack. |
Hi @bretg !
next:
So, the changes:
|
Thanks @rpanchyk - updated. |
Discussed and approved in PBS PMC |
This is done in PBS-Java. Assigning to @hhhjort for PBS-Go implementation. |
For the record, here are the PBS-Java PRs that implemented events: prebid/prebid-server-java#296 |
This is largely complete in PBS-Go. @laurb9 noted two remaining things to implement:
@danielguedesb Are you able to continue contributing to add those areas of functionality? |
This older ticket did not define the entire functionality needed for events and I can't find another one that does (edit: or maybe #1470 ), so I will add it here from https://docs.google.com/document/d/1ry0X4C2EV-R0pMrm1IQk9BstxaT395UCl3KKqTGa5c8/edit# so we are all on the same page. Let me know if this should be another ticket. I'm starting work on this.
|
I think we're good with continuing the discussion here.
Sweet. Thank you. |
I've implemented most of the logic in #1597 . The goal was to get the unit tests to a point where we can validate the behavior, then do some refactoring. Some of the tests are currently failing in a non-significant way because of timestamp mismatch (until #1584 is merged) but are covering the behavior we need. I see a few differences between what PBS-Java does and the algorithm above:
|
Let's discuss the revised algorithm presented by @laurb9 above at the prebid.org committee meeting this Friday. |
The algorithm says "If ext.prebid.cache.vastXml is specified, then cache the VAST in PBS. Otherwise, just return the modified VAST." So this is a PBS-Java bug. Opened an internal ticket to resolve. Heads up @rpanchyk However, I did notice that the algorithm summary didn't check that events were enabled for video before adding the tag. Updated to note that "If events are enabled for this account". See https://docs.google.com/document/d/1ry0X4C2EV-R0pMrm1IQk9BstxaT395UCl3KKqTGa5c8/edit#
Algorithm summary fixed.
Another PBS-Java ticket opened to confirm. Thanks.
Integration is defined in #1428 |
I'm going to work on updating the VAST everywhere in #1597 as well, in that case. To do that, I think I might need some clarification on expected behavior for the case below: when What should happen when events are enabled and |
More questions: Should
|
Thanks for pushing this @laurb9 - this is quite intricate. I've revised (somewhat heavily) the Events doc. I don't think the algorithm has changed much, but trying to structure to document to make the various constraints more clear.
So specifically answering your questions:
@rpanchyk - while researching this in PBS-Java code, I reviewed the "analytics_config" database column as implemented and have questions.
But our database refers to
|
hi, @bretg
|
I've tried to make a truth table to clarify some of the situations. How does this look ?
|
@bretg I've had a few more questions while working on adding the unit tests based on the truth table above:
I do not see where this happened, maybe I am looking at a cached doc version ? request.ext.prebid.events only impacts non-video bids as far as I can tell.
I assume that is a qualified "always" because of this in the algorithm
Does this override the account settings ? Should this be host-controlled, if they implemented pacing in some way since it's not part of PBS ?
|
- 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)
prebid#1015 (comment) - all VIDEO bids, cached and returned, have modified VAST if account and bidder enabled - not just winning cached bids. - all NON-VIDEO returned bids have `ext.prebid.events` if account or request enabled - all cached NON-VIDEO bids have `wurl` patched in if account or request enabled.
prebid#1015 (comment) - all VIDEO bids, cached and returned, have modified VAST if account and bidder enabled - not just winning cached bids. - all NON-VIDEO returned bids have `ext.prebid.events` if account or request enabled - all cached NON-VIDEO bids have `wurl` patched in if account or request enabled.
I've updated the PR to what I think is a potentially releasable functionality. I see exchange code is a refactor target, so I'd like to defer the additional PG and channels functionality as an incremental update for later, to avoid more PR conflicts |
- Add validation for Video bids. `bid.adm` or `bid.nurl` needs to be present - This case now, is not possible `<VASTAdTagURI><![CDATA[null]]></VASTAdTagURI>` - Bid adm will be updated as cache. (see prebid/prebid-server#1015) - Bid type is defined by bidder, not our (`imp.video != null`, etc) checks. For example Appnexus use `bid.ext.appnexus.bid_ad_type` to define it. (that's why there are a lot of changed cache jsons. Also add ordering for tests) Refactored a bit. - Removed confusing maps - Removed confusing checks - Removed several imp to bid matching Possible improvements (in another ticket bc current PR is too large) - Extract more event URL to another class - Use bidInfo in BidResponseCreator for BidResponse
@laurb9 - created a somewhat modified version of your truth table at the bottom of https://docs.google.com/document/d/1ry0X4C2EV-R0pMrm1IQk9BstxaT395UCl3KKqTGa5c8/edit# . The only comment for your version:
Responding to questions in your comment:
right - that was the original set up -- the proposal here is that request.ext.prebid.events overrides the server account event/analytics config for video as well as banner/native.
Removed the word "always".
PG not supported in PBS-Go.
The idea is that request.ext.prebid.events is a request-level override to the server-side config.
PG may not be built in PBS-Go. That would be a fairly major effort and ought to be driven by demand for the feature. Channels -- PBS-Java has new account level "analytics_config" that defines analytics on a per-channel basis. Unfortunately I don't think the JSON attribute very well named. Currently it's "auction-events", but it's really more a generic flag for analytics support by channel. Will open a separate discussion on whether we want to make changes there. #1470 |
VAST is modified if *either* account or request setting demanded it. prebid#1015 (comment)
I updated the code and tests so Please review the unit tests at cached wurl tests are in |
@laurb9 We've merged your events PR. Would you say that PBS-Go now implements this feature or are there still gaps? |
Out of the things specced in the document, we are only missing PG and integration channels. PG was said to be not implemented yet in PBS-Go, so that part can be added when PG functionality is worked on. Integration channels: PBS-Go has a per-account enable flag for events. PBS-Java has that, plus three additional subflags that can control it for amp, app and web individually for each account if set. This was mentioned here. This, I have not implemented because I was left the impression that channel mapping needed further discussion (#1428, #1470), and the channel wasn't readily available in the auction for me to use. |
* Add modifying of VAST for video bids and add validation - Add validation for Video bids. `bid.adm` or `bid.nurl` needs to be present - This case now, is not possible `<VASTAdTagURI><![CDATA[null]]></VASTAdTagURI>` - Bid adm will be updated as cache. (see prebid/prebid-server#1015) - Bid type is defined by bidder, not our (`imp.video != null`, etc) checks. For example Appnexus use `bid.ext.appnexus.bid_ad_type` to define it. (that's why there are a lot of changed cache jsons. Also add ordering for tests) Refactored a bit. - Removed confusing maps - Removed confusing checks - Removed several imp to bid matching Possible improvements (in another ticket bc current PR is too large) - Extract more event URL to another class - Use bidInfo in BidResponseCreator for BidResponse * Merged master and fixed corresponding imp can't be null * Fix after review. (without changing localhost/event) * Use placeholders in test resources instead of concrete urls where possible (#1084) * Fix tests and remove ordering * Replace usage of Wiremock EqualToJsonPattern with custom implementation to prevent incorrect json comparison * fix openx * Use equalToBidCacheRequest for consistency and add openrtbCacheDebugComparator() when debug is used (fix flaky tests) * Remove redundant code Co-authored-by: Sergii Chernysh <[email protected]> Co-authored-by: rpanchyk <[email protected]>
Updated VAST updating algorithm |
* Add modifying of VAST for video bids and add validation - Add validation for Video bids. `bid.adm` or `bid.nurl` needs to be present - This case now, is not possible `<VASTAdTagURI><![CDATA[null]]></VASTAdTagURI>` - Bid adm will be updated as cache. (see prebid/prebid-server#1015) - Bid type is defined by bidder, not our (`imp.video != null`, etc) checks. For example Appnexus use `bid.ext.appnexus.bid_ad_type` to define it. (that's why there are a lot of changed cache jsons. Also add ordering for tests) Refactored a bit. - Removed confusing maps - Removed confusing checks - Removed several imp to bid matching Possible improvements (in another ticket bc current PR is too large) - Extract more event URL to another class - Use bidInfo in BidResponseCreator for BidResponse * Merged master and fixed corresponding imp can't be null * Fix after review. (without changing localhost/event) * Use placeholders in test resources instead of concrete urls where possible (#1084) * Fix tests and remove ordering * Replace usage of Wiremock EqualToJsonPattern with custom implementation to prevent incorrect json comparison * fix openx * Use equalToBidCacheRequest for consistency and add openrtbCacheDebugComparator() when debug is used (fix flaky tests) * Remove redundant code Co-authored-by: Sergii Chernysh <[email protected]> Co-authored-by: rpanchyk <[email protected]>
* Add modifying of VAST for video bids and add validation - Add validation for Video bids. `bid.adm` or `bid.nurl` needs to be present - This case now, is not possible `<VASTAdTagURI><![CDATA[null]]></VASTAdTagURI>` - Bid adm will be updated as cache. (see prebid/prebid-server#1015) - Bid type is defined by bidder, not our (`imp.video != null`, etc) checks. For example Appnexus use `bid.ext.appnexus.bid_ad_type` to define it. (that's why there are a lot of changed cache jsons. Also add ordering for tests) Refactored a bit. - Removed confusing maps - Removed confusing checks - Removed several imp to bid matching Possible improvements (in another ticket bc current PR is too large) - Extract more event URL to another class - Use bidInfo in BidResponseCreator for BidResponse * Merged master and fixed corresponding imp can't be null * Fix after review. (without changing localhost/event) * Use placeholders in test resources instead of concrete urls where possible (#1084) * Fix tests and remove ordering * Replace usage of Wiremock EqualToJsonPattern with custom implementation to prevent incorrect json comparison * fix openx * Use equalToBidCacheRequest for consistency and add openrtbCacheDebugComparator() when debug is used (fix flaky tests) * Remove redundant code Co-authored-by: Sergii Chernysh <[email protected]> Co-authored-by: rpanchyk <[email protected]>
* Add modifying of VAST for video bids and add validation - Add validation for Video bids. `bid.adm` or `bid.nurl` needs to be present - This case now, is not possible `<VASTAdTagURI><![CDATA[null]]></VASTAdTagURI>` - Bid adm will be updated as cache. (see prebid/prebid-server#1015) - Bid type is defined by bidder, not our (`imp.video != null`, etc) checks. For example Appnexus use `bid.ext.appnexus.bid_ad_type` to define it. (that's why there are a lot of changed cache jsons. Also add ordering for tests) Refactored a bit. - Removed confusing maps - Removed confusing checks - Removed several imp to bid matching Possible improvements (in another ticket bc current PR is too large) - Extract more event URL to another class - Use bidInfo in BidResponseCreator for BidResponse * Merged master and fixed corresponding imp can't be null * Fix after review. (without changing localhost/event) * Use placeholders in test resources instead of concrete urls where possible (#1084) * Fix tests and remove ordering * Replace usage of Wiremock EqualToJsonPattern with custom implementation to prevent incorrect json comparison * fix openx * Use equalToBidCacheRequest for consistency and add openrtbCacheDebugComparator() when debug is used (fix flaky tests) * Remove redundant code Co-authored-by: Sergii Chernysh <[email protected]> Co-authored-by: rpanchyk <[email protected]>
This issue will be closed, as everything requested here has been implemented besides PG. And since PG is a big feature, we decided to branch that off into it's own issue #2312 |
As a followup to the event tracking issue #800, we're ready to implement the injection of additional tracking into VAST XML. Initially this is just to count impressions, but will be expanded to additional video events at some point. There are two scenarios, both of which are required and could be happening at the same time:
Here's a flow diagram showing how tracking will work. Changed items are in red.
Proposed modifications
We propose a new bidder configuration property
modifyingVastXmlAllowed
, with the default being false.The contents of the <impression> tag are pulled from a new event.url-template property that has macros that need to be resolved. e.g.
where b=BIDID, a=ACCOUNT
Scan the 'adm' on video bids:
Once we know what type of VAST is present:
Unlike server-side video, the VAST XML coming into the browser didn't go through Prebid Server, so will not have the tracking strings added. Prebid.js has configuration that allows the publisher to initiate "client-side caching". In order to modify the VAST XML, the page will refer to a new end point on Prebid Server that will perform the same modifications as if it had come through the server.
Where /vtrack is a new endpoint that causes the request to go to PBS where the steps 1-3 above will be applied.
If the
vasttrack
parameter is true, Prebid.js will add a couple of parameters to the POST XML to the specified endpoint with this JSONPBS doesn't currently have a /vtrack endpoint -- we shouldn't use /cache because PBC implements that one. The new endpoint would use a similar code path as for server-side VAST:
POST the modified VAST to Prebid Cache, wait for the results from PBC, and forward them to the client. e.g.
{"responses":[{"uuid":"94531ab8-c662-4fc7-904e-6b5d3be43b1a"}]}
PBS should validate the arguments supplied with /vtrack: if ACCOUNT, BIDID, or BIDDER aren't supplied, it should reject the /vtrack request with 400. This should get caught when the page is being tested.
If the ACCOUNT doesn't exist and PBS is enforcing accounts, that should also result in a 400.
PBS will process /vtrack similar to the server-side VAST response, inserting impression tracking when:
Closed Item:
The text was updated successfully, but these errors were encountered: