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

Emit event when setTargetingForGPTAsync is called #873

Merged
merged 5 commits into from
Jan 19, 2017

Conversation

itayo155
Copy link
Contributor

Type of change

  • Feature

Description of change

Emit a new event ('setTargeting') when setTargetingForGPTAsync is called. This indicates that all bids have been collected and actually sent away.
For reference regarding why this is needed and intended use, please see issue #858.

Other information

Original issue: #858

@itayo155
Copy link
Contributor Author

Following up - any comments, or thoughts when it could be merged?

@protonate protonate self-assigned this Jan 3, 2017
@mkendall07
Copy link
Member

@itayo155
can you rebase off master to resolve conflicts?

@itayo155
Copy link
Contributor Author

itayo155 commented Jan 4, 2017

Sure, done. Please let me know if there are any other comments.

@jaiminpanchal27 jaiminpanchal27 added this to the Prebid 0.18.0 milestone Jan 6, 2017
@jaiminpanchal27
Copy link
Collaborator

@itayo155
I don't think we should add this "SET_TARGETING" event for 2 reasons

  1. We will be adding another targeting function for Appnexus adserver and maybe in future some more. So we should not be emitting this event from all the other public functions.
  2. It is very similar to AUCTION_END event. Hence if its unreliable, we should fix that instead of adding new similar event.

@itayo155
Copy link
Contributor Author

itayo155 commented Jan 17, 2017

@jaiminpanchal27
OK. If it is agreed that the AUCTION_END event should be emitted when bidding ends (i.e. when additional bids will not participate in current auction anymore), I suggest to close this PR and open a new one, for "making the AUCTION_END more accurate" or something like that.

However, given the way things are usually set-up by publishers, I see only two ways of implementing this change:

  1. Hook the individual setTargeting functions (e.g. for DFP, AN, etc.), similarly to the way its done in the current PR. If there are multiple setTargeting functions expected, this can be done by calling a single endpoint which would emit the event from each of them.
  2. Force a change in the way publishers interact with Prebid, e.g. by changing the API.

The reason is that setTargetging is usually being called directly from publisher's code, and more specifically - this happens when the auction times out at a time arbitrarily selected by publisher.
E.g. (adapted from a live implementation, comments are mine):

//start the auction
  pbjs.que.push(function() {
      pbjs.addAdUnits(adUnits);
      //...
      pbjs.requestBids({
          bidsBackHandler: sendAdserverRequest
      });

//set a timeout - arbitrarily selected by publisher
 setTimeout(function() {
      sendAdserverRequest();
  }, some_publisher_set_timeout);

//end the auction - note the setTargetingForGPTAsync is being called directly; and only at this point the auction actually ends
  function sendAdserverRequest() {
    //...      
        googletag.cmd.push(function() {
          pbjs.que.push(function() {
              //auction ends now, and we'd like the AUCTION_END event to somehow be emitted
              pbjs.setTargetingForGPTAsync();
              //...
          });
      });
  }

Would be happy for additional ideas for how to implement this.
Out of the two options above, I personally find (1) preferable.

Would be happy to hear your thoughts.

@mkendall07
Copy link
Member

@itayo155
Thanks for the PR. We'll review internally and get back to you.
Could you also send me an email directly? My email is in my profile. Thanks

@jaiminpanchal27
Copy link
Collaborator

@itayo155
Looks good. Merging the PR.

@jaiminpanchal27 jaiminpanchal27 merged commit 65d1d22 into prebid:master Jan 19, 2017
Walexander pushed a commit to MbidIO/Prebid.js that referenced this pull request Mar 6, 2017
* emitting event when setTargetingForGPTAsync is called

* emitting event when setTargetingForGPTAsync is called - cleanup

* emitting event when setTargetingForGPTAsync is called - cleanup
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.18.0 to release/1.13.0

* commit 'e145489bc5dd6d292cf16e7fe80adfdf991562ac': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…13.0 to master

* commit '7d32ed18c8636d9241ef8b299b6abd885536db69': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
@itayo155 itayo155 deleted the adding-settargeting_event branch June 11, 2017 11:36
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.

4 participants