Skip to content
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

Feature Request: expose an interface to inject impression trackers into VAST bids #9085

Closed
matthieularere-msq opened this issue Oct 6, 2022 · 13 comments

Comments

@matthieularere-msq
Copy link
Contributor

Type of issue

Question

Description

Is it expected that an analytic module is able to alter a bid response ? I found out it could do so.
I want to be able to use this but first I would need to be sure this is expected, allowed and won't be removed later.

Thanks.

Steps to reproduce

In file modules/bidwatchAnalyticsAdapter.js add the following starting line 95 :

  if (args.ad !== undefined)
    args.ad = "<!-- removed -->";

Then the creative code sent by bidders is replaced by this and no ad is shown on the page.

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 6, 2022

We have some precedent of using event handlers as hooks for modifying data, so it's not necessarily disallowed, but without knowing more about the use case I'd find it surprising behavior if it comes from analytics.

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 6, 2022

Should what you are trying to do be in an RTD module? see https://docs.prebid.org/dev-docs/module-rules.html#analytics-adapter-rules

@bretg
Copy link
Collaborator

bretg commented Oct 6, 2022

@matthieularere-msq - analytics adapters can't mess with the bids. This is a job for a a real-time-data module... https://docs.prebid.org/dev-docs/add-rtd-submodule.html

@matthieularere-msq
Copy link
Contributor Author

I understand analytics adapters are not supposed to do that and that's why I wondered. However what I am willing to do is related with analytics as I wished to insert video impression tracking in vast bids. I see I can do it, but I am unsure if it's ok to do so.

Before creating a reat-time-date submodule to supplement my analytic one, could you confirm me weither this would be accepted in an analytics module or not :

bid.vastImpUrl = bid.vastUrl !== undefined
  ? bid.vastImpUrl !== undefined
  ? getImpUrl(bid) + '&url=' + encodeURI(bid.vastImpUrl)
  : getImpUrl(bid)
  : bid.vastUrl;
// Vast XML document
if (bid.vastXml !== undefined) {
  const doc = new DOMParser().parseFromString(bid.vastXml, 'text/xml');
  const wrappers = doc.querySelectorAll('VAST Ad Wrapper, VAST Ad InLine');
  if (wrappers.length) {
	wrappers.forEach(wrapper => {
	const impression = doc.createElement('Impression');
	impression.appendChild(doc.createCbidSection(getImpUrl(bid)));
	wrapper.appendChild(impression)
	});
	bid.vastXml = new XMLSerializer().serializeToString(doc);
  }
}

I somehow feel in any case it's a rough solution: doing it in analytic adapter kind of break the rules but it's done for analytics purposes. But doing it by a rtd submodule makes analytics depend of another module while it should be a all-in-one solution.

Anyway, I believe that if analytics are not supposed to alter bids in any way, then it would be safier for them to receive derefenced copy of objects instead of the ones they currently get.

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 7, 2022

I am unsure of whether that pattern would be OK in an analytics adapter. (@patmmccann ?)
But I agree that either option is not ideal. Here's a couple ideas for improvement:

  • Prebid core - or possibly the new video module - could expose an interface for interested parties to inject their impression trackers into VAST bids; or
  • Prebid could manage its own centralized impression tracker (probably through Prebid Server, similar to how it does video caching), inject it into VAST bids, and use it to generate normal BID_WON events.

@patmmccann
Copy link
Collaborator

I really like the idea of analytics adapters receiving dereferenced copies; that would seem to break this other code proposal.

Would that cause a substantial performance overhead?

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 7, 2022

Would that cause a substantial performance overhead?

Yes if we try to do it with any significant degree of exhaustiveness - it'd require a deep clone of almost every object.

@patmmccann patmmccann changed the title analytics module and altering creatives Feature Request: expose an interface to inject impression trackers into VAST bids Oct 11, 2022
@patmmccann
Copy link
Collaborator

@matthieularere-msq looks like this isn't going to fly from the analytics adapter, however, we're adding the injection interface to the feature request backlog so let's keep this one open

@patmmccann
Copy link
Collaborator

closing #9055 to consolidate discussion

@dgirardi
Copy link
Collaborator

Proposal:

  • analytics adapters can define a method getVASTTrackers(bidRespone)
  • publishers can allow it with pbjs.enableAnalytics([{allowVASTInjection: true}])
  • if allowed, core will inject impression trackers in VAST bids

@bretg
Copy link
Collaborator

bretg commented Oct 25, 2022

How about instead of letting the analytics adapter update the VAST themselves, we just get an array of URLs and core adds the tags?

Letting them update it themselves is powerful, but we might end up with duplicate logic for injecting all of the other trackers.

@jimdigriz
Copy link
Contributor

jimdigriz commented Nov 4, 2022

...we did this as an AdServer module, it includes functionality to massage the VAST tag into a blob URI to avoid refetching it.

@fowler446
Copy link
Collaborator

Closing this for now as it's quite old and isn't a priority at the moment.

@github-project-automation github-project-automation bot moved this from Ready for Dev to Done in Prebid.js Tactical Issues table Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

6 participants