-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
PBjs Core : add ability to inject tracking in video #10191
Changes from 8 commits
e407ca2
14d1d3b
643c74a
e70f141
2019c74
53f94b3
e182c18
2587ba6
4f8086f
94bbe1f
1934a1b
4a1eeaf
1138443
a7d72f5
2c18305
ecd61fe
641c597
cb593d5
b74c5c6
8083d25
98f5d80
84d039c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,7 @@ import {config} from './config.js'; | |
import {userSync} from './userSync.js'; | ||
import {hook} from './hook.js'; | ||
import {find, includes} from './polyfill.js'; | ||
import {OUTSTREAM} from './video.js'; | ||
import {OUTSTREAM, getVastTrackers, insertVastTrackers} from './video.js'; | ||
import {VIDEO} from './mediaTypes.js'; | ||
import {auctionManager} from './auctionManager.js'; | ||
import {bidderSettings} from './bidderSettings.js'; | ||
|
@@ -582,7 +582,9 @@ function tryAddVideoBid(auctionInstance, bidResponse, afterBidAdded, {index = au | |
}), 'video'); | ||
const context = videoMediaType && deepAccess(videoMediaType, 'context'); | ||
const useCacheKey = videoMediaType && deepAccess(videoMediaType, 'useCacheKey'); | ||
const [hasTrackers, vastTrackers] = getVastTrackers(bidResponse); | ||
|
||
if (hasTrackers) { bidResponse.vastXml = insertVastTrackers(vastTrackers, bidResponse.vastXml); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's make this dependent on configuration as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something like that to add in config ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @patmmccann , can you confirm me that I understood what you asked for so I can make the changes ? Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any feedback on this please ? |
||
if (config.getConfig('cache.url') && (useCacheKey || context !== OUTSTREAM)) { | ||
if (!bidResponse.videoCacheKey || config.getConfig('cache.ignoreBidderCacheKey')) { | ||
addBid = false; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,6 +21,8 @@ export const videoBidder = bid => includes(adapterManager.videoAdapters, bid.bid | |||||
export const hasNonVideoBidder = adUnit => | ||||||
adUnit.bids.filter(bid => !videoBidder(bid)).length; | ||||||
|
||||||
let vastTrackers = []; | ||||||
|
||||||
/** | ||||||
* @typedef {object} VideoBid | ||||||
* @property {string} adId id of the bid | ||||||
|
@@ -64,3 +66,47 @@ export const checkVideoBidSetup = hook('sync', function(bid, adUnit, videoMediaT | |||||
|
||||||
return true; | ||||||
}, 'checkVideoBidSetup'); | ||||||
|
||||||
export function registerVASTTrackers(tracker) { | ||||||
dgirardi marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
vastTrackers.push(tracker); | ||||||
} | ||||||
|
||||||
export function insertVastTrackers(trackers, vastXml) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this logic already exists in the Video Module Library. Why not reuse existing code ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't aware of this library. I would say not reuse this existing code avoids to run |
||||||
const doc = new DOMParser().parseFromString(vastXml, 'text/xml'); | ||||||
karimMourra marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const wrappers = doc.querySelectorAll('VAST Ad Wrapper, VAST Ad InLine'); | ||||||
try { | ||||||
if (wrappers.length) { | ||||||
wrappers.forEach(wrapper => { | ||||||
trackers['impressions'].forEach(trackingUrl => { | ||||||
const impression = doc.createElement('Impression'); | ||||||
impression.appendChild(doc.createCDATASection(trackingUrl)); | ||||||
wrapper.appendChild(impression) | ||||||
}); | ||||||
}); | ||||||
vastXml = new XMLSerializer().serializeToString(doc); | ||||||
karimMourra marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} catch (error) { | ||||||
logError('an error happened trying to insert trackers in vastXml'); | ||||||
} | ||||||
return vastXml; | ||||||
} | ||||||
|
||||||
export function getVastTrackers(bid) { | ||||||
let hasTrackers = false; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this implementation needs to be cleaned, it's not easy to follow and is difficult to manage. A few things that could help:
Lastly, the interface is hard to work with. I have to look at the implementation to understand what it's returning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Latest commit made changes which should match those you asked for. Can you tell me if there is something else you would like to see modify here or if I didn't understood what you requests ? I was not quite sure about the |
||||||
let trackers = {'impressions': []}; | ||||||
vastTrackers.forEach(func => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the elements in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here is an example of implementation at the analytic module side :
The function registered in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also the reason why |
||||||
let tmpTrackers = func(bid); | ||||||
for (const key in tmpTrackers) { | ||||||
if (key in trackers && Array.isArray(tmpTrackers[key])) { | ||||||
// only add not existing trackers | ||||||
tmpTrackers[key].forEach(item => { | ||||||
if (!trackers[key].includes(item)) { | ||||||
trackers[key].push(item); | ||||||
hasTrackers = true; | ||||||
} | ||||||
}); | ||||||
} | ||||||
} | ||||||
}); | ||||||
return [hasTrackers, trackers]; | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -12,6 +12,7 @@ | |||||
import {ajaxBuilder} from './ajax.js'; | ||||||
import {config} from './config.js'; | ||||||
import {auctionManager} from './auctionManager.js'; | ||||||
import {getVastTrackers} from './video.js'; | ||||||
|
||||||
/** | ||||||
* Might be useful to be configurable in the future | ||||||
|
@@ -42,17 +43,22 @@ const ttlBufferInSeconds = 15; | |||||
* @param {string} impUrl An impression tracker URL for the delivery of the video ad | ||||||
* @return A VAST URL which loads XML from the given URI. | ||||||
*/ | ||||||
function wrapURI(uri, impUrl) { | ||||||
function wrapURI(uri, impUrl, arrayVastTrackers) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to include the type in the name
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
// Technically, this is vulnerable to cross-script injection by sketchy vastUrl bids. | ||||||
// We could make sure it's a valid URI... but since we're loading VAST XML from the | ||||||
// URL they provide anyway, that's probably not a big deal. | ||||||
let vastImp = (impUrl) ? `<![CDATA[${impUrl}]]>` : ``; | ||||||
let impressions = (impUrl) ? `<Impression><![CDATA[${impUrl}]]></Impression>` : ``; | ||||||
if (Array.isArray(arrayVastTrackers) && arrayVastTrackers.length == 2 && arrayVastTrackers[1].hasOwnProperty('impressions')) { | ||||||
arrayVastTrackers[1]['impressions'].forEach(trackingImp => { | ||||||
impressions += `<Impression><![CDATA[${trackingImp}]]></Impression>`; | ||||||
}); | ||||||
} | ||||||
return `<VAST version="3.0"> | ||||||
<Ad> | ||||||
<Wrapper> | ||||||
<AdSystem>prebid.org wrapper</AdSystem> | ||||||
<VASTAdTagURI><![CDATA[${uri}]]></VASTAdTagURI> | ||||||
<Impression>${vastImp}</Impression> | ||||||
${impressions} | ||||||
<Creatives></Creatives> | ||||||
</Wrapper> | ||||||
</Ad> | ||||||
|
@@ -67,7 +73,7 @@ function wrapURI(uri, impUrl) { | |||||
* @param index | ||||||
*/ | ||||||
function toStorageRequest(bid, {index = auctionManager.index} = {}) { | ||||||
const vastValue = bid.vastXml ? bid.vastXml : wrapURI(bid.vastUrl, bid.vastImpUrl); | ||||||
const vastValue = bid.vastXml ? bid.vastXml : wrapURI(bid.vastUrl, bid.vastImpUrl, getVastTrackers(bid)); | ||||||
const auction = index.getAuction(bid); | ||||||
const ttlWithBuffer = Number(bid.ttl) + ttlBufferInSeconds; | ||||||
let payload = { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to work with functions that don't return standard types. In this case it appears to return an array where the first element is a bool, and the 2nd is an object.
Instead, why not return null when there are no trackers ? The type is then a nullable object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done