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

clearTargeting #391

Closed
CarsonBanov opened this issue Jun 6, 2016 · 19 comments
Closed

clearTargeting #391

CarsonBanov opened this issue Jun 6, 2016 · 19 comments
Assignees
Labels

Comments

@CarsonBanov
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@brondsem
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link

gramorris commented Jun 15, 2016

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
Copy link

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
Copy link
Contributor

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
Copy link
Member

@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
Copy link

Thanks Matt, I'll start a new issue

@mkendall07
Copy link
Member

@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
Copy link
Member

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.

@protonate protonate added the bug label Jun 16, 2016
@mkendall07
Copy link
Member

This is resolved with #415

@brondsem
Copy link
Contributor

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

@jalbersmead
Copy link

@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
Copy link

gramorris commented Jul 27, 2016

@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
Copy link
Member

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

@gramorris
Copy link

@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
#484

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

Successfully merging a pull request may close this issue.

6 participants