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

Fix: Incorrect generation of analytics impression url when bid response is a VAST URL (videoTrackers.js) #12252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shubham-si
Copy link
Contributor

@shubham-si shubham-si commented Sep 18, 2024

Type of change

  • Bugfix

Issue

The issue arises when bid response has vastUrl as video response. Impression urls for analytics are registered via vastTrackers.registerVastTrackers function. This module gets impression urls on bidResponse and concatenate to bidResponse.vastImpUrl as Set<Array<string>>.

i-2

The videoCache.js module then add these impression urls to vast. It requires impression urls as string|string[].
The current implementation for generating CDATA blocks in <Impression> tags is incorrectly handling impression trackers when the video response is a vastUrl. Instead of embedding the actual URLs, the output is showing [object Set].

Code Ref. :

impTrackerURLs ? impTrackerURLs.map(trk => `<Impression><![CDATA[${trk}]]></Impression>`).join('') : '';

The code is meant to generate impression URLs by mapping over impTrackerURLs, which it assume is Array<string>. However, instead of producing URLs as expected, the output inside the tags is [object Set] when it should be the actual URLs as strings.

cache

This issue will impact video ad tracking, leading to incorrect reporting and analytics.

Steps to reproduce

  • Register vastTrackers function as mentioned here with any analytics adapter eg., medianetAnalytics

  • It can be a single url of type event: impressions

    registerVastTrackers(MODULE_TYPE_ANALYTICS, 'test', function(bidResponse) {
      return [
        {'event': 'impressions', 'url': `https://vasttracking.mydomain1.com/vast?cpm=${bidResponse.cpm}`}
      ];
    });
  • Or can be multiple url's of type event: impressions
    registerVastTrackers(MODULE_TYPE_ANALYTICS, 'test', function(bidResponse) {
      return [
        {'event': 'impressions', 'url': `https://vasttracking.mydomain1.com/vast?cpm=${bidResponse.cpm}`},
        {'event': 'impressions', 'url': `https://vasttracking.mydomain2.com/vast?cpm=${bidResponse.cpm}`}
      ];
    });

issue

Solution

Convert the Set<string> to an array before concatenating to bidResponse.vastImpUrl.

  • With videoResponse as vastUrl
    after fix - vastUrl

  • With videoResponse as vastXml
    after fix - vastXml

…ckers.js module.Getting object Required Array<string>.
@shubham-si shubham-si changed the title Fixed issue of adding analytics impression pixel via Prebid's vastTra… Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module.Getting object Required Array Sep 18, 2024
@shubham-si shubham-si changed the title Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module.Getting object Required Array Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module. Getting object Required Array Sep 18, 2024
@shubham-si shubham-si changed the title Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module. Getting object Required Array Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module. Getting object Required Array<String> Sep 18, 2024
@shubham-si shubham-si changed the title Fixed issue of adding analytics impression pixel via Prebid's vastTrackers.js module. Getting object Required Array<String> vastTrackers module : Incorrect CDATA Generation of Impression Trackers for Video having vastUrl as response. Getting Set<Array.String> Required Array<String>. Sep 18, 2024
@shubham-si shubham-si changed the title vastTrackers module : Incorrect CDATA Generation of Impression Trackers for Video having vastUrl as response. Getting Set<Array.String> Required Array<String>. Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Getting Set<Array.string> Required string|Array for videoCache module. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Getting Set<Array.string> Required string|Array for videoCache module. Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Getting Set<Array.string> Required string|Array<string>. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Getting Set<Array.string> Required string|Array<string>. Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect formation of analytics impression url's for video responses via vastTrackers.js module. Fix: Incorrect concatenation of analytics impression url's to video vastImpUrl via videoTrackers.js module. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect concatenation of analytics impression url's to video vastImpUrl via videoTrackers.js module. Fix: Incorrect generation of analytics impression url's to video vastImpUrl via videoTrackers.js module. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect generation of analytics impression url's to video vastImpUrl via videoTrackers.js module. Fix: Incorrect generation of vast impression trackers having vastUrl as video response via videoTrackers.js module. Sep 18, 2024
@shubham-si shubham-si changed the title Fix: Incorrect generation of vast impression trackers having vastUrl as video response via videoTrackers.js module. Fix: Incorrect generation of analytics impression url when bid response is a VAST URL (videoTrackers.js) Sep 18, 2024
@shubham-si shubham-si marked this pull request as ready for review September 18, 2024 13:07
@shubham-si
Copy link
Contributor Author

Hey @patmmccann , could you please check this MR since you have defined the spec.

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 1, 2024

I tried adding a test case for this but I am unable to update this PR.

Could you either add a test yourself, or merge in ab3bb90 ? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants