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

Support request level Bidder Params #1042

Closed
PubMatic-OpenWrap opened this issue Sep 24, 2019 · 16 comments
Closed

Support request level Bidder Params #1042

PubMatic-OpenWrap opened this issue Sep 24, 2019 · 16 comments
Labels
Intent to implement An issue describing a plan for a major feature. These are intended for community feedback

Comments

@PubMatic-OpenWrap
Copy link
Contributor

PubMatic-OpenWrap commented Sep 24, 2019

Reopening issue for #626

A few Bidders now have defined params that aren't really "per-Imp" params. Appnexus has member, and Pubmatic has wrapper param.

There may be others too. It might be a good idea to generalize support for bidder params in the request.ext, rather than forcing them all to be in the imp.

@PubMatic-OpenWrap
Copy link
Contributor Author

PubMatic-OpenWrap commented Sep 24, 2019

This is the approach that we propose to add request specific bidder parameters in the oRTB request.

Currently, the Ext object in the BidRequest is openrtb_ext.ExtRequest

// ExtRequest defines the contract for bidrequest.ext
type ExtRequest struct {
        Prebid ExtRequestPrebid json:"prebid"
}

The proposal is to add another parameter 'BidderParams', which would contain bidder specific data, to ExtRequestPrebid

// ExtRequestPrebid defines the contract for bidrequest.ext.prebid 
type ExtRequestPrebid struct { 
  Aliases               map[string]string       json:"aliases,omitempty"
  BidAdjustmentFactors  map[string]float64      json:"bidadjustmentfactors,omitempty"
  Cache                 *ExtRequestPrebidCache  json:"cache,omitempty"
  StoredRequest         *ExtStoredRequest       json:"storedrequest,omitempty"
  Targeting             *ExtRequestTargeting    json:"targeting,omitempty"
  BidderParams          interface{}             json:"bidderparams,omitempty"   ==> new parameter 
}

BidderParams interface would be a map, with key as bidderName and value would be another map containing bidder specific data.

Example:

var BidderParams map[BidderName]BidderSpecificParams 
type BidderSpecificParams map[string]interface{}

Each adapter can/should define what the bidder specific data (BidderSpecificParams) would be in order for them to retrieve that information from the request
so BidderParams in the request that prebid would receive would look like

"bidderparams": { 
  "adapter1": { 
    "key1": "data specific to key1", 
    "key2": "  { 
      "version": 3.3, 
       "profileid": 1234 
      } 
    }, 
  "adapter2": { 
   "key3": "data specific to key3" 
  }, 
  "adapter2": { 
    "key4": [ 
     "data" 
    ] 
  } 
} 

This data can be filtered when the request is split for each bidder so that request that is sent to the adapter only has the data specific to that adapter

So request going to adapter1 will have

request: { 
  "ext" : { 
   "prebid": { 
     "bidderparams": { 
        "key1": "data specific to key1", 
        "key2": "  { 
          "version": 3.3, 
          "profileid": 1234 
        } 
      } 
    } 
  } 
} 

Request going to adapter2 will have

request: { 
  "ext" : { 
    "prebid": { 
      "bidderparams": { 
       "key3": "data specific to key3" 
      } 
    } 
  } 
} 

Request going to adapter3 will have

request: { 
  "ext" : { 
    "prebid": { 
      "bidderparams": {    
       "key4": [ 
          "data" 
        ] 
      } 
    } 
  } 
} 

There was no entry for adapter4 in the bidderparams, so request going to adapter4 will have

request: { 
  "ext" : { 
    "prebid": { } 
  } 
} 

This filtration, of sending only bidder specific data within the request to that specific bidder, can be done in the splitBidRequest function in exchange/utils.go.

Since each adapter has defined the data structure that they expect, they can retrieve it easily when a request is sent to them and use the data as per their requirement.

I am not sure about how to validate this bidder specific data yet. One thought is to have the BidderParams structure for each adapter defined in the bidder schema so it can be validated along with all the other request parameters or to have the same standard for everyone where each adapter can have BidderParams as just key/value pairs where the key is a string and value is an array of strings

Please let us know your thoughts regarding this approach. We have done similar changes in our fork to accommodate for request specific bidder data. We can raise a PR and discuss the changes there if that seems like the more appropriate place for this discussion

@bretg
Copy link
Contributor

bretg commented Sep 24, 2019

So to be clear, the OpenRTB request coming into the server would carry the additional info on ext.prebid.bidderparams?

{
"ext": {
  "prebid": {
    "bidderparams": { 
       "adapter1": { 
          "key1": "data specific to key1", 
          "key2": "  { 
            "version": 3.3, 
             "profileid": 1234 
            } 
          }, 
      "adapter2": { 
         "key3": "data specific to key3" 
      }, 
      "adapter2": { 
          "key4": [ 
            "data" 
          ] 
       } 
    }
  }
} 

This works well with Prebid.js, which allows for setting of ext.prebid in s2sConfig:

pbjs.setConfig({
   s2sConfig: {
      extPrebid: {
          bidderparams: {
             ...
          }
      }
   }
});

So this proposal works for me. Thanks for bringing it up. Again. @hhhjort - good with you guys?

@bretg bretg added the Intent to implement An issue describing a plan for a major feature. These are intended for community feedback label Sep 24, 2019
@hhhjort
Copy link
Collaborator

hhhjort commented Sep 24, 2019

Mostly ok with it. I ran into a possible stumbling block when considering the question on how to validate ... each bidder publishes a bidder params json schema that their bidder params must adhere to. So it seems an obvious move to publish a second json schema for request level parameters. But then, at least for a transition phase, you have some ambiguity as to what belongs where.

Consider the case where property A is required in the imp level schema, but should go to the request level. The way to transition this is make A optional in the imp schema, and optional in the request schema. That way both the old and the new locations are valid locations. Unfortunately this also means omitting A entirely is also valid according to the schema, but is a requirement for the adapter. During this interim phase, the adapter will have to check both locations for A, and error out if it is not found in either location. Also leads to undefined behavior if different values for A are found in the two locations.

I don't think this should be fatal to the proposal, but we should spend some thought on any ways to mitigate this migration issue.

@pm-deepak-dhage
Copy link

@bretg , and @hhhjort, Thanks for the review and feedback.
To respond to concern raised by @hhhjort , "Also leads to undefined behaviour if different values for A are found in the two locations."
I would say, adapter needs to define a priority if there is same parameter present at impression as well as request level. I think, impression level property should override the one at request level, but it totally depends on implementor to define the behaviour and document it for clarity.

@hhhjort
Copy link
Collaborator

hhhjort commented Nov 1, 2019

As mentioned earlier, I favor merging the two sources of parameters in order to enforce the parameters schema. To support the ability to have the adapter choose the priority order of the two sources, I would recommend merging the two with imp level priority in each imp, and passing the request level parameters at that level untouched. This lets adapters take advantage of the request level parameters without doing any actual work (publishers do not have to confirm an adapter supports request level parameters before using them) while adapters that want the request level to take priority can simply override the imp level with the request level. And we can still easily enforce the schemas on the parameters.

There are two corner cases where schema enforcement will fail. One is where the imp level value breaks the schema, but request level value satisfies the schema. If the adapter gives priority to the request level, then technically it would pass, but PBS would consider the parameters as failing with the default imp priority. I am good with that, as the publisher has specified an invalid value, even though overall the invalid value does not get used.

The other is where the imp level is good, but the global is invalid, and the adapter gives priority to global. PBS will pass the schema check, but the adapter must be aware that it should vet the global parameters it is overriding with, as they may not have been validated. An adapter level check will only invalidate the adapter participating, rather than failing the entire request as the PBS level schema check will do. I don't really see a solution to this beyond having PBS check both ways, but I don't think the extra overhead is worth it. The adapter could fall back to the imp level parameter if the request level is invalid, or just not process the request at their option.

@bretg
Copy link
Contributor

bretg commented Nov 4, 2019

To support the ability to have the adapter choose the priority order of the two sources, I would recommend merging the two with imp level priority in each imp

To be clear, you recommend having PBS core merge the ext.prebid.bidderparams into every imp[*].ext.prebid.BIDDER, right?

@hhhjort
Copy link
Collaborator

hhhjort commented Jan 10, 2020

That is correct

@bretg
Copy link
Contributor

bretg commented Jan 17, 2020

@PubMatic-OpenWrap - are you good with this? Basically, it's the best of both worlds: you can specify parameters globally in ext.prebid.bidderparams.pubmatic, and PBS core will copy them to imp[*].ext.prebid.pubmatic so adapters don't need to be modified.

If you like this, are you up for coding it, or willing to wait for when it hits the PBS-Go team's priority queue?

@PubMatic-OpenWrap
Copy link
Contributor Author

Hi @bretg , this looks good. We'll take this up for coding. Thank you!

@SyntaxNode
Copy link
Contributor

@PubMatic-OpenWrap Is this still something you plan to work on?
@bretg Do you think this would this affect the First Party Data feature?

@pm-deepak-dhage
Copy link

@SyntaxNode, Yes, it is still in plan. We will have a PR by end of Jan/early Feb.

@bretg
Copy link
Contributor

bretg commented Dec 11, 2020

This is still fine -- doesn't affect first party data.

@bretg
Copy link
Contributor

bretg commented Jan 4, 2021

Got a question from the PBS-Java engineering team that highlights the need for more clarification.

Should request level parameters be merged to all impressions, even if impression does not have such bidder in imp.ext or only matching bidders between imp and request levels should be merged?

My take is that this is related tangentially to issue #1478 -- a bid adapter receives its imp-level parameters on imp[].ext.prebid.bidder (formerly imp[].ext.bidder)

So I think it should receive the request level params on ext.prebid.bidderparams.bidder -- our rule has been that an adapter cannot see parameters meant for other other adapter.

If ext.prebid.bidderparams.adapter1 is specified, I think server should merge to imp[].ext.prebid.adapter1 whether or not it exists initially. Then it can be passed through validation. Adapters are not required to have imp-level params. Nor are they required to have global params. (there's at least one adapter that doesn't take any params at all)

Also, in reviewing this, found a typo in the 2nd comment above:

  "adapter2": { 
    "key4": [ 
     "data" 
    ] 
  } 
} 

This should be "adapter3" instead to match the example output shown in "Request going to adapter3 will have"

@SyntaxNode
Copy link
Contributor

Implemented in PBS-Go 0.184.0

@sachin-pubmatic @hhhjort Is this fully implemented in PBS-Go, or what aspects of this specification are still remaining?

@hhhjort
Copy link
Collaborator

hhhjort commented Jan 10, 2022

I believe this is now implemented in PBS-Go. I am not sure if anyone has used the merge feature in production yet. (The request bidder level parameters are also available in their request level position.)

@bretg
Copy link
Contributor

bretg commented Feb 7, 2022

done in PBS-Java v1.74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Intent to implement An issue describing a plan for a major feature. These are intended for community feedback
Projects
None yet
Development

No branches or pull requests

5 participants