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

initial update for bidder schain support - feedback wanted #4479

Closed
wants to merge 1 commit into from

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Feature

Description of change

This is a Work In Progress for bidder specific schain changes.

I would like to get feedback on this initial approach, which is a bit different that what was proposed in #4465.

Summary of module (in this draft state):

  • after the adapterManager.makeBidRequests function has executed, the module will be called to process the schain config(s).
  • while going through each bidRequest object (one for each bidder), the module will 'look-up' the schain config for that bidder. It will attempt to find a bidder specific one if it exists, otherwise it'll assign the global config.
  • During each schain look-up, the module will evaluate the schain based on it's validation setting and either set schain.config object on the bidRequest.bids.schain field. This field will not be set If either: the validation is strict and the schain config is setup incorrectly or the overall schain object was not an object.
  • After populating each of the bidRequests, the auction will resume as normal. Bidders will read this bidRequest schain field (as they did before) and set it in their request.

What's different against the proposal:

  • using a different hook point.
  • the module is still passing the bidder's schain in an auction object for them to read later.
  • bidders are still looking at a bidRequest's schain field instead of calling config.getConfig('schain'); (so that prebid will dynamically pull the bidder's schain).

The main reasons for this shift was: a set of limitations I encountered when attempting to invalidate an schain object within the Prebid config objects and how making bidders use the config.getConfig with some type of a workaround for the limitations would feel. See details below...

Troubles on overwriting the Prebid Configs

I tried to overwrite the schain config by re-executing the setBidderConfig (and setConfig) in the following manners:

setBidderConfig({
  bidder: ['appnexus'],
  config: {
    // schain: null,
    // schain: {},
    schain: 'invalid'
  }
});

In the null case - it passed through the setBidderConfig function and made it null. But when I called the config.getConfig in the bidder - it always pulled the global schain config instead of just null. This was due to this logic check here; it thought there was no bidderConfig defined for this topic, so it fell back to the global.

In the {} case - the overwrite didn't work in the setBidderConfig due to the Object.assign here; since the existing bidder config was listed before the new config (and there were no new values to overwrite), the bidder config stayed put.

In the 'invalidstring case - this actually worked in both thesetBidderConfigand in theconfig.getConfig` in the bidder. But this didn't seem to be ideal way to manage the implementation for the bidders.

Using config listeners to evaluate the config as its submitted

I also attempted to look into using the subscribe() and listeners functionality from the config.js file to do the config evaluation right after it was registered by the pbjs.setConfig(); this is done by registering a callback function using `config.getConfig('module', function()...); in the module file.

While this approach loaded the global schain config for us to read, this subscribe/listener functionality is not currently setup for the setBidderConfig. Given the limited functionality, this approach didn't seem like it would pan-out.

Inserting schain in auction object again for bidders

Given the results above and that we couldn't cleanly overwrite the prebid configs, I thought to continue using the approach to pass in a value in the bidRequests for the bidder to read.

I toyed with some flags that would set for each bidder so they'd know to call the config.getConfig, but this didn't really make sense and looked like an extra step the bidder had to go through. The module was already capable of finding the correct schain config for that bidder (whether it was the global or an override), so it could easily pass the correct config to that bidder in a controlled, secure manner.

This final approach is what I implemented (currently) in this draft PR as it seemed to be a more ideal setup.

If you got this far - thanks for reading through the above. :) If you have any suggestions or other ideas to try out so that we're more in-line with the original proposal, please feel free to share.

Thanks.

@jaiminpanchal27
Copy link
Collaborator

We discussed this approach offline. Until we find some solution for troubles which @jsnellbaker already mentioned above, this approach looks good to me.

@snapwich
Copy link
Collaborator

I submitted a fix for the null overrides. I'm fine with this pull-request as is, or if bidderConfig can fulfill the need now that you can set null, then I'm fine going that route as well.

@bretg
Copy link
Collaborator

bretg commented Nov 25, 2019

Where are we with this one @jsnellbaker ?

@jsnellbaker
Copy link
Collaborator Author

Going to back to this task now and will try to wrap it up. Based on some offline conversations, I'll continue with this updated approach to reduce the impact to bidders. I'll update the I2I as well to conform.

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