-
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
Add support for instream video ads bidsWon tracking #5481
Conversation
3fb7ffb
to
4de1586
Compare
@bretg @Fawke @jaiminpanchal27 can you please review this PR and let me know your thoughts ? |
@mike-chowla what do you think about this approach?
|
@monisq can you make the changes to core for the (suggested in review) so that BID_WON event will be available for all for instream video bids. |
Hello @monisq , |
Yes, I am already working on it! would be pushing the changes by 2mrow! |
Thank you @monis0395 👍 |
@pm-harshad-mane I have done the changes in the core, can please review them? Till then, I am trying figuring out, how to write unit tests for instream tracking (facing some issues while mocking and stubing), will push those changes as soon as done |
Thank you @monis0395 !! |
@pm-harshad-mane I have added unit test cases for instream tracking. Do you think we should change the title or PR as its a little different from initial PR ? |
@monis0395 @jsnellbaker |
Done! |
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.
LGTM.
Requesting second review from @jsnellbaker .
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.
Hi @monis0395
I had a few questions. When you have the chance, can you please take a look at them?
Additionally, I'm going to tag some others to review this PR as well. There was something similar to this effort (properly tracking winning instream bids) before and there were some concerns raised at the time. I don't recall the specifics currently, I will try to dig up the PR in question.
Thanks.
We're not in a position to 'enforce' much. This feature has to be considered optional.
It could be documented. e.g. "if you use spotx, add ____ to the regex."
Are you with a Prebid.org member company? The Prebid.js or Video Taskforce committee meetings would be good forums. At a high level I view this as a fine optional feature, but bidsWon for video is a metric of questionable value IMO -- kind of a "poor-man's" metric since advertisers pay for video on impression and unlike banner, there could be a large time difference in video between ad call and actual display. Rubicon has already built a video impression-tracking feature that's working well. And tracking for App and AMP. See these references:
The issue has been that PBS-Go doesn't support events (for video specifically, the /vtrack endpoint). The good news here is that we may have some community support for porting that feature from PBS-Java to PBS-Go. |
@monis0395 , can you please move this code to a separate module and provide some documentation? |
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.
If this is broken out into a separate module I'll approve it. But the documentation has to mention the scenario where this method of counting could break down, basically unexpected URLs carrying the cache ID.
Thanks for waiting, I have done the suggested code changes! I have started working on prebid.github.io doc changes for this PR, would be pushing those in a day or two. Thanks! |
Great @monis0395 !! Please mention the documentation PR here when you are ready. |
Hey @bretg @pm-harshad-mane , Here is the documentation PR : prebid/prebid.github.io#2277 |
qq: should this affect https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L764 ; which seems to mark winning bids as used too aggressively ? |
As such this PR wont affect |
@jsnellbaker @idettman could you please review the changes? |
Hi @jsnellbaker @bretg, Did you get a chance to look at it? |
Hi @bretg, as you have suggested. I have already broken down in a separate module.. The docs PR is also approved by you prebid/prebid.github.io#2277 Let me know if anything else needs to be done! Thanks! |
Hello @bretg , |
My review is merely for positioning and user-facing concerns. Since we have two code reviews done already, I've removed Isaac from the list. My one question about merging is whether this will be in 4.8 or if that train has already left. |
Thank you @bretg , I have merged it to master, so it will be available in 4.8, doc PR need to be merged post 4.8 release. |
Thank you @monisq for your efforts and patience. 👍 |
Thanks @pm-harshad-mane 👍 |
Type of change
Description of change
Problem:
Possible Solution for #4768, #5308 :
Since we know for sure every type of bid response will have videoCacheKey and will be loaded by VideoPlayer. We can use the Performance API to keep track of any resource fired with a video cache key.