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

Rubicon. bugfix for singleRequest config regression #9050

Merged

Conversation

elad-yosifon
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

please see #9049

@ChrisHuie ChrisHuie changed the title fix rubicon singleRequest config regression #9049 Rubicon. bugfix for singleRequest config regression Sep 29, 2022
@ChrisHuie ChrisHuie self-assigned this Sep 29, 2022
@ChrisHuie ChrisHuie merged commit f32e080 into prebid:master Sep 29, 2022
@elad-yosifon elad-yosifon deleted the fix_rubicon_single_request_config branch October 23, 2022 12:02
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
* fix rubicon singleRequest config regression prebid#9049

* manually kick off testing

Co-authored-by: Chris Huie <[email protected]>
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
* fix rubicon singleRequest config regression prebid#9049

* manually kick off testing

Co-authored-by: Chris Huie <[email protected]>
@robertrmartinez
Copy link
Collaborator

robertrmartinez commented Aug 8, 2023

@elad-yosifon @ChrisHuie

Not sure how this got in and merged without someone from Magnite / Rubicon reviewing or commenting at least.

But I fail to see how this was not working?

We do keep an ongoing internal cache of any update made to the rubicon config.

Unless you are using prebid in a different way, where you load prebid core first, then run all your que block setConfigs, and then initialize other modules.

Because the que block should be executed AFTER modules are loaded. Including our bid adapter.

And since that is the case, our module is initialized and should register to the config BEFORE anything in the queue block does.

You can test this by putting breakpoints in your first que block function and inside of a module.

The code before the change you implemented:

let rubiConf = {};
// we are saving these as global to this module so that if a pub accidentally overwrites the entire
// rubicon object, then we do not lose other data
config.getConfig('rubicon', config => {
  mergeDeep(rubiConf, config.rubicon);
});


   // ..... and in bidRequest time
   if (rubiConf.singleRequest !== true) {

This saves any setConfig rubicon call into rubiConf. and this if statement will evaluate to FALSE and hit the else statement if you do your example:

pbjs.que.push(() => {
  pbjs.setConfig({
    rubicon: {
      singleRequest: true
    }
  });
});

It 100% does run singleRequest auctions and keeps the state of what the above setConfig implements. Since que blocks are executed after modules have been initialized.

Here are some screenshots which prove this (I added info logs to show how this works)

image

And we see only one call to Rubicon Fastlane endpoint:

image

And here is the reverse where we turn singleRequest off

image

image

In fact this PR introduces what I consider a bug, and the whole reason we added this logic in the commit you reference as introducing the bug.

With the change implemented by this PR the following would turn OFF single request auctions

pbjs.que.push(() => {
  pbjs.setConfig({
    rubicon: {
      singleRequest: true
    }
  });

  pbjs.setConfig({
    rubicon: {
      netRevenue: true
    }
  });
});

Which is obviously NOT intended.

These cases where multiple setConfigs for our rubicon object happen is exactly why we introduced the saving of the internal state using the rubiConf code I posted above.

This was before the adoption of pbjs.mergeConfig which I still see rarely used by publishers. Which is why we still maintain our internal cache of a self merged rubiConf.

Please, if you are going to propose a PR for an adapter, the maintainers should be tagged first and given some time to comment and review the PR.

If I am mistaken and you can provide an example test page where my change would not work please share.

But from my testing it works just fine.

Here is a jsfiddle which points to Prebid 7.18.0 which is the version before this PR was merged.

https://jsfiddle.net/0khqzt15/12/

I am planning to revert this PR back to the old implementation.

I am not aware that there is a use case where some pbjs.setConfigs can be executed BEFORE modules are loaded, however, I will add in logic to handle this in the PR I will submit.

This line of code should handle if this is a valid use case.

let rubiConf = pbjs.getConfig('rubicon') || {};

CC @bretg @harpere

@robertrmartinez
Copy link
Collaborator

Here is the PR #10332

This PR reverts back to the old method I had set, as well as solve for any potential use case where a rubicon config is somehow set before rubicon module is initialized.

ChrisHuie pushed a commit that referenced this pull request Aug 9, 2023
santii7395 pushed a commit to themaven-net/Prebid.js that referenced this pull request Aug 28, 2023
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 this pull request may close these issues.

None yet

3 participants