prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.28k stars 2.05k forks source link

clearTargeting #391

Closed CarsonBanov closed 8 years ago

CarsonBanov commented 8 years ago

Due to the recent efforts to enable refresh through Prebid this line of code: https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L110 has been added. The line encapsulates the notion that Prebid should clear existing targeting before re-setting it from the new bids. However, it is too heavy-handed in the case that users are either 1) using code/scripts outside of Prebid for header-bidding or 2) are setting their own custom slot-level targeting. What should actually happen is that Prebid should call clearTargeting("key") multiple times where "key" is all of the Prebid targeting KVP. As it exists currently, other non-Prebid targeting will be wiped out!

Example: a user sets some targeting name: adBanner on their first ad slot. On the first refresh this will be removed by the call to clearTargeting and that refreshed ad impression will not have the targeting that the publisher expects.

.cc @mkendall07 @nanek @mmilleruva

CarsonBanov commented 8 years ago

I couldn't find a way to do this, if you provide a clear method for clearing just the Prebid targeting I can make a PR

protonate commented 8 years ago

The use case you describe is accommodated by presetTargeting. Before setting Prebid targeting, any targeting already set (e.g. name: adBanner) is stored in presetTargeting here: https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L135

Then resetPresetTargeting clears all targeting then replaces the preset targeting here: https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L113

Are you seeing incorrect behavior for this?

CarsonBanov commented 8 years ago

I'll check and re-open if I see any incorrect behavior. Thanks.

brondsem commented 8 years ago

I am also seeing targeting being lost. I am new to prebid.js so may be doing something wrong. But it looks like requestBids calls resetPresetTargeting, so as soon as you do that, your targeting is lost. And presetTargeting from within getWinningBidTargeting hasn't run yet so no custom targeting is saved yet.

mkendall07 commented 8 years ago

@brondsem Can you explain the sequence of events on your page or provide code? I'm specifically interested in why DFP is being called before setting the targeting?

brondsem commented 8 years ago

We've taken the current simple example (with "instant load") and integrated it to our site. There's a lot of other resources (and ad setup) on our site, but I think the problem comes down to timing in which the googletag.cmd queue happens to run before the pbjs.que. If we remove the async attr on prebid script tag, then it forces it to load sooner and pbjs.que items run first.

I've tried to replicate this in a jsfiddle, but I think part of the problem is all the other resources on our page causing scripts to load slower or in nondeterministic order. I've taken the sample fiddle and made some changes, including adding a 200 ms delay around addAdUnits and requestBids in an attempt to simulate the actual delays: http://jsfiddle.net/7d956kt0/3/

Perhaps something is needed to synchronize the two queues, similar to how sendAdserverRequest does?

gramorris commented 8 years ago

Hi,

I believe we seeing behaviour related to this too. We use the disableInitialLoad on GPT feature but we also load ads up in groups depending on the device, so we load a leadboard and an mpu in the head as they exist on all devices and then on desktop we might load an additional mpu and a sky ad as they are drawn on screen.

In 0.8.1 this behaviour works fine, however in 0.9.2 the leaderboard in failing in renderAd with "ERROR: Error trying to write ad. Cannot find ad by given id : 31fe525abd64c6" and the MPU is not having it's bid parameters passed. I can see the two slots having their key values reset immediately after the additional slots are invoked with pbjs.addAdUnits

screen shot 2016-06-15 at 12 55 00 screen shot 2016-06-15 at 12 54 40

gramorris commented 8 years ago

Further to my previous comment this behaviour is coming about as the reset calls here: https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L443

It appears to me that the behaviour here is slightly presumptive that you would only call requestBids once. If you call it more than once there's no guarantee that previous requests have been completed since pbjs.clearAuction() is called in the bids back callback but the _bidsRequested could be used in a renderAd coming back from DFP at some time in the future.

e.g. requestBids #1 -> bidsReceived -> clearAuction -> send request to DFP -> requestBids #2 -> bidsRequested cleared -> DFP returns from request #1 and calls renderAds -> Error as bidsRequested has been cleared

brondsem commented 8 years ago

For my issue, I believe this is a regression from bf06c0b as the original reporter mentioned it adds the clearTargeting call. It is not related to GPT instant load, as I first thought. The commit with clearTargeting was added in 0.9.2 which is where the issue starts for me; I can use 0.9.1 and targetting params are not lost.

mkendall07 commented 8 years ago

@gramorris I think that might be a separate issue than what is described in this thread.

@brondsem & @CarsonBanov we'll take a look at your code.

gramorris commented 8 years ago

Thanks Matt, I'll start a new issue

mkendall07 commented 8 years ago

@brondsem Having difficulty replicating the bad behavior. I think you might be correct that there is nondeterministic behavior with clearing the slots at the time of requestBids. We'll look into moving that into the setTargetingForGPTAsync instead...

mkendall07 commented 8 years ago

Found the issue. The slots are cleared initially if DFP is loaded first with this line here: https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L110

Fix in the works.

mkendall07 commented 8 years ago

This is resolved with https://github.com/prebid/Prebid.js/pull/415

brondsem commented 8 years ago

Thanks, latest version from master seems to be working for me.

jalbersmead commented 8 years ago

@gramorris Did you create a new issue for what you were seeing or did you find a resolution? I'm seeing the same thing in 0.11.0 that you were

gramorris commented 8 years ago

@jalbersmead I didn't in the end as we were going to refactor some of our code to account for it. I'm happy to start on though and be a part of the discussion.

mkendall07 commented 8 years ago

@jalbersmead Can you open a new ticket with a detailed description to of the issue for replication? Thank you.

gramorris commented 8 years ago

@mkendall07 @jalbersmead I've tested with 0.11.0 to make sure it's still present as we've experienced it and created an issue https://github.com/prebid/Prebid.js/issues/484