Vast trackers: additional event injection feature#14409
Vast trackers: additional event injection feature#14409vivekyadav15 wants to merge 6 commits intoprebid:masterfrom
Conversation
…vast-tracker-feature
…vast-tracker-feature
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b5fe9eb45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function getVastXml(bid) { | ||
| return bid.vastXml ? bid.vastXml : wrapURI(bid.vastUrl, bid.vastImpUrl); | ||
| return bid.vastXml ? bid.vastXml : wrapURI(bid.vastUrl, bid.vastTrackers); |
There was a problem hiding this comment.
Preserve vastImpUrl when wrapping vastUrl bids
This call only passes bid.vastTrackers into wrapURI, so bids that still populate the legacy vastImpUrl (which multiple adapters in this repo do) will no longer get their impression trackers injected when the bid only has vastUrl. That’s a behavior regression for existing adapters unless they’re all updated to emit vastTrackers. Consider merging vastImpUrl into vastTrackers.impression (or passing both) to keep backward compatibility.
Useful? React with 👍 / 👎.
Pull Request Test Coverage Report for Build 21713757215Details
💛 - Coveralls |
| if (isValidVastTracker(trackers, trackerToAdd)) { | ||
| trackers.push(trackerToAdd); | ||
|
|
||
| if (isPlainObject(trackersToAdd)) { |
There was a problem hiding this comment.
This is not backwards compatible is it? since there's only one user IMO it's worth refactoring it as part of this.
Prebid.js/modules/medianetAnalyticsAdapter.js
Lines 215 to 220 in cceccba
| * VAST trackers to attach to this bid (impression, error, and tracking events). | ||
| */ | ||
| vastImpUrl?: string | string [] | ||
| vastTrackers?: VastTrackers |
There was a problem hiding this comment.
There's more adapters that are populating vastImpUrl - either they should be refactored or this should keep accepting it
There was a problem hiding this comment.
-
We could introduce a fallback (
vastImpUrl) for backward compatibility. However, this would require maintaining two separate mechanisms for the same feature. To minimize confusion and reduce long-term maintenance overhead, I recommend standardizing on the newvastTrackersapproach instead of supporting both. -
To ensure the latest changes related to
vastImpUrlare consistently applied across all impacted components, we can update the following files:- targetVideoUtils/bidderUtils.js
- adrelevantisBidAdapter.js
- appnexusBidAdapter.js
- mediafuseBidAdapter.js
- medianetAnalyticsAdapter.js
Please let me know if you have a different opinion or any concerns.
Summary
Adds support for analytics adapters to register and inject three VAST tracker types:
<Impression><![CDATA[url]]></Impression><Error><![CDATA[url]]></Error><TrackingEvents><Tracking event="..."><![CDATA[url]]></Tracking></TrackingEvents>Analytics adapter contract
registerVastTrackers(MODULE_TYPE_ANALYTICS, name, trackerFn)trackerFn(bid, {auction, bidRequest})returns:How it works for
vastXml(direct XML injection)If the bid already has
bidResponse.vastXml, we parse it and append:<Impression>nodes forimpression[]<Error>nodes forerror[]<Tracking>nodes under<Linear><TrackingEvents>fortrackingEvents[]If
<TrackingEvents>(or<Linear>) is missing, the code creates the required nodes before appending tracking events.How it works for
vastUrl/VAST URI(wrapper flow)If the bid does not have
vastXmland instead uses avastUrl, the video cache wrapper generates a Wrapper VAST XML that:<VASTAdTagURI>to the bidder’s VAST URISo trackers are applied in both cases:
This PR highlights the issue #14353.