Pass Host Config Info to Bid Adapters #2370

AlexBVolcy · 2022-09-06T16:17:46Z

This PR addresses this issue: #1739

The goal of the PR is to pass specific host info to bid adapters in two different ways

This host info data we want passed consists of: externalUrl, gvlID, and datacenter

One way is to pass this info in the auction request itself at the path ext.prebid.server (i.e. ext.prebid.server.datacenter)

The second way is to pass this info as a configuration data structure

When it comes to adding the info to the auction request, all bid adapters get a copy of the request, so as long as I properly update the request to include these new values (and ensure they don't get cleaned), this should suffice in accomplishing the first way of passing the info.

To do this, what I needed was to create a new request object in openrtb_ext/request.go called ExtRequestPrebidServer, which will hold the three host info values we want to support. This object is then added as another parameter of the ExtRequestPrebid object.

To ensure the new host info wouldn't be scrubbed from the request that's sent to bid adapters, I had to update cleanOpenRTBRequests() --> buildRequestExtForBidder() and add the new host info Server object to the ExtRequestPrebid object that's being built there.

When it comes to passing this info as config data structure, I created a new data structure in config.go called Server which holds the three host info values, and that object is added to the Configuration object. And then I updated SetupViper() so that the host could set the values of this object.

In order for this config object to be available to adapters, I had to update the Builder() function that all adapters use to build a new instance of their adapter. Simply, the Builder() function is updated to accept the config Server object (which is taken from the Server object defined by Viper), and that Server object is then added to that particular adapter object.

This change had to be applied to all adapters, and all adapters tests, so that's where the bulk of the files changed comes from.

SyntaxNode · 2022-09-06T17:12:42Z

adapters/33across/33across.go

@@ -15,6 +15,7 @@ import (

 type TtxAdapter struct {
 	endpoint string
+	Server   config.Server


You're not going to like this after already completing the work, but I recommend not to add the server variable to the adapter structs. Let's leave it to adapters to capture that information in the future if they'd like. For now, I think we're good with just the updates to the Builder methods.

No worries, I've got the muscle memory down for updating the adapters! I'll remove this

config/config.go

SyntaxNode · 2022-09-06T17:18:16Z

openrtb_ext/request.go

@@ -57,7 +57,8 @@ type ExtRequestPrebid struct {
 	// NoSale specifies bidders with whom the publisher has a legal relationship where the
 	// passing of personally identifiable information doesn't constitute a sale per CCPA law.
 	// The array may contain a single sstar ('*') entry to represent all bidders.
-	NoSale []string `json:"nosale,omitempty"`
+	NoSale []string                `json:"nosale,omitempty"`
+	Server *ExtRequestPrebidServer `json:"server,omitempty"`


Please add this alphabetically in the above section.

SyntaxNode

Looks good. Thank you for the updates. I noticed two more things in this review pas.

exchange/utils.go

SyntaxNode · 2022-09-19T21:13:24Z

exchange/adapter_util.go

@@ -26,7 +27,7 @@ func BuildAdapters(client *http.Client, cfg *config.Configuration, infos config.
 	return exchangeBidders, nil
 }

-func buildBidders(adapterConfig map[string]config.Adapter, infos config.BidderInfos, builders map[openrtb_ext.BidderName]adapters.Builder) (map[openrtb_ext.BidderName]adapters.Bidder, []error) {
+func buildBidders(adapterConfig map[string]config.Adapter, infos config.BidderInfos, builders map[openrtb_ext.BidderName]adapters.Builder, hostInfo config.Server) (map[openrtb_ext.BidderName]adapters.Bidder, []error) {


I recommend to rename hostInfo to server to match the name in the Builder definition and to consider keeping the builders argument as the last one in the signature.

bsardo · 2022-09-20T14:47:05Z

config/config.go

@@ -597,6 +598,12 @@ type Debug struct {
 	OverrideToken       string              `mapstructure:"override_token"`
 }

+type Server struct {
+	ExternalUrl string `mapstructure:"external_url"`
+	GdprID      int    `mapstructure:"gdpr_id"`


Nitpick: Can we change this name to indicate it is a GVL ID or the GDPR host vendor ID so it is more indicative of what it represents? This data structure appears to be internal to PBS so it should be ok to deviate from what is described in the issue.

bsardo · 2022-09-20T16:07:41Z

openrtb_ext/request.go

@@ -108,6 +109,12 @@ type ExtRequestPrebidCache struct {
 	VastXML *ExtRequestPrebidCacheVAST `json:"vastxml"`
 }

+type ExtRequestPrebidServer struct {
+	ExternalUrl string `json:"externalurl"`
+	GdprID      string `json:"gvlid"`


Can we call this something else like GVLID, HostGVLID or GDPRHostVendorID instead of GdprID to more accurately reflect what this is? I see this is referred to as Gdpr ID in the issue but changing this variable name shouldn't have any external effect since the name of the field on a request is gvlid and the config value is gdpr.host_vendor_id.

bsardo · 2022-09-26T18:43:51Z

exchange/exchange.go

@@ -202,6 +204,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 	if err != nil {
 		return nil, err
 	}
+	requestExt.Prebid.Server = &openrtb_ext.ExtRequestPrebidServer{ExternalUrl: e.server.ExternalUrl, GvlID: e.server.GvlID, Datacenter: e.server.Datacenter}


Should we only be setting this if we have at least one piece of server data? Otherwise we end up with an empty prebid.server object in our bid requests.

Probably a good idea. An Empty method on ExtRequestPrebidServer might be nice to check for this condition.

That's a good check to add! Will update

SyntaxNode · 2022-09-26T19:57:05Z

config/config.go

@@ -95,7 +95,7 @@ type Configuration struct {
 	HostSChainNode    *openrtb2.SupplyChainNode `mapstructure:"host_schain_node"`
 	// Experiment configures non-production ready features.
 	Experiment Experiment `mapstructure:"experiment"`
-
+	Datacenter string     `mapstructure:"datacenter"`


Nitpick: Consider changing name to DataCenter. The Viper name and mapstructure should stay as is.

SyntaxNode · 2022-09-26T19:57:42Z

config/config.go

+type Server struct {
+	ExternalUrl string `mapstructure:"external_url"`
+	GvlID       int    `mapstructure:"gvlid"`
+	Datacenter  string `mapstructure:"datacenter"`


Nitpick: Consider changing name to DataCenter. You can use the rename functionality in your editor to do this easily.

SyntaxNode · 2022-09-26T19:59:45Z

config/config.go

+type Server struct {
+	ExternalUrl string `mapstructure:"external_url"`
+	GvlID       int    `mapstructure:"gvlid"`
+	Datacenter  string `mapstructure:"datacenter"`


Nitpick: Please remove the mapstructure attributes. Those are for viper and we're not using viper here.

SyntaxNode · 2022-09-26T20:00:49Z

config/config.go

@@ -729,7 +735,7 @@ func (cfg *Configuration) AccountDefaultsJSON() json.RawMessage {
 	return cfg.accountDefaultsJSON
 }

-//Allows for protocol relative URL if scheme is empty
+// Allows for protocol relative URL if scheme is empty


Nipick: If you're going to fix the comment, please change it to start with the function name, ie
// GetBaseURL allows for...

SyntaxNode · 2022-09-26T20:02:17Z

openrtb_ext/request.go

+type ExtRequestPrebidServer struct {
+	ExternalUrl string `json:"externalurl"`
+	GvlID       int    `json:"gvlid"`
+	Datacenter  string `json:"datacenter"`


Nitpick: Consider changing field name to DataCenter.

bsardo · 2022-09-27T18:23:04Z

exchange/exchange.go

+	if requestExt.Prebid.Server.Empty() {
+		requestExt.Prebid.Server = &openrtb_ext.ExtRequestPrebidServer{ExternalUrl: e.server.ExternalUrl, GvlID: e.server.GvlID, DataCenter: e.server.DataCenter}
+	}


Ah my earlier thought here wasn't about checking if there was any server data specified in the request, but rather about checking if there was any host data so that we only set requestExt.Prebid.Server if e.server is non-empty in order to avoid creating an object at ext.prebid.server without any real data in it.

My understanding is that server info specified in the host config should always overwrite what is specified in the request.

@SyntaxNode, your suggestion was that it might make sense to put an Empty method on ExtRequestPrebidServer. Were you thinking the logic would then be something like this?

serverInfo := openrtb_ext.ExtRequestPrebidServer{ExternalUrl: e.server.ExternalUrl, GvlID: e.server.GvlID, DataCenter: e.server.DataCenter} if !extPrebidServer.Empty() { requestExt.Prebid.Server = &serverInfo }

Or should the Empty method go on type Server defined in config/config.go instead?

Oh yes, I think I messed this up! I think you're right in saying we should only set requestExt.Prebid.Server here when e.server is non-empty. And I could add an Empty method to check if e.server is non-empty.

bsardo

LGTM, but need confirmation from @SyntaxNode regarding handling an empty host config.

exchange/exchangetest/aliases.json

bsardo · 2022-10-06T16:04:50Z

exchange/utils_test.go

@@ -2369,74 +2369,28 @@ func TestBuildRequestExtForBidder(t *testing.T) {
 			expectedJson:         json.RawMessage(`{"prebid":{}}`),
 		},
 		{
-			description:          "Prebid - Allowed Fields Only",


I think you might have made a mistake when merging with master. The test cases in this test shouldn't have been deleted or modified to remove references to the test input alternateBidderCodes.
I think you might want to just augment some of these test cases by adding references to request.ext.prebid.server. Actually I think you had done this but lost it during the merge.

AlexBVolcy added 4 commits August 31, 2022 13:33

Init commit, all adapters builder() updated

4535deb

Host config variable name change

cfee749

Update adapter tests to support new host info

b59535d

Update cleanOpenRTB so server info is available

5ef8c1e

AlexBVolcy changed the title ~~Pass Host Config to Bid Adapters~~ Pass Host Config Info to Bid Adapters Sep 6, 2022

SyntaxNode reviewed Sep 6, 2022

View reviewed changes

SyntaxNode self-assigned this Sep 6, 2022

bsardo self-requested a review September 6, 2022 17:20

bsardo self-assigned this Sep 6, 2022

AlexBVolcy added 3 commits September 12, 2022 09:26

Remove server variable from adapter structs

f0ecfc2

Change alphabetical order

30d9011

Server struct copy values from config, test update

26fffd8

SyntaxNode reviewed Sep 19, 2022

View reviewed changes

bsardo reviewed Sep 21, 2022

View reviewed changes

AlexBVolcy added 2 commits September 21, 2022 15:02

Variable renaming, update tests

c187b1e

Merge branch 'master' into pass_host_config

cbe13f2

bsardo previously approved these changes Sep 22, 2022

View reviewed changes

bsardo self-requested a review September 22, 2022 15:57

Ensure server info is set if from host config

13ff9c1

AlexBVolcy dismissed bsardo’s stale review via 13ff9c1 September 23, 2022 23:35

bsardo reviewed Sep 26, 2022

View reviewed changes

SyntaxNode reviewed Sep 26, 2022

View reviewed changes

Address minor comments, add empty method

f4d6446

bsardo reviewed Sep 27, 2022

View reviewed changes

AlexBVolcy added 3 commits September 27, 2022 18:12

Alter empty method

f443bf2

Test fix

d221991

Update test coverage

3450bca

bsardo previously approved these changes Sep 28, 2022

View reviewed changes

SyntaxNode reviewed Oct 4, 2022

View reviewed changes

exchange/exchangetest/aliases.json Show resolved Hide resolved

Beautify json files

4dbf97e

AlexBVolcy dismissed bsardo’s stale review via 4dbf97e October 5, 2022 04:07

AlexBVolcy added 4 commits October 5, 2022 09:55

Merge branch 'master' into pass_host_config

cc404e6

One test fix

ef59e70

Merge branch 'master' into pass_host_config

70e23bf

Fix error created during merge

6a90b63

bsardo reviewed Oct 6, 2022

View reviewed changes

Re-added tests lost in merge, and server ref

debfd34

bsardo approved these changes Oct 6, 2022

View reviewed changes

SyntaxNode approved these changes Oct 12, 2022

View reviewed changes

AlexBVolcy merged commit c2e6c25 into prebid:master Oct 12, 2022

This was referenced Oct 12, 2022

New Adapter: Bidstack #2416

Merged

New Adapter: Clickonometrics #2410

Merged

New Adapter: VideoHeroes #2399

Merged

New Adapter: Brave #2396

Merged

New Adapter: freewheelssp #2392

Merged

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Pass Host Config Info to Bid Adapters (prebid#2370)

4b1b737

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Host Config Info to Bid Adapters #2370

Pass Host Config Info to Bid Adapters #2370

AlexBVolcy commented Sep 6, 2022 •

edited

Loading

SyntaxNode Sep 6, 2022

AlexBVolcy Sep 8, 2022

SyntaxNode Sep 8, 2022

SyntaxNode Sep 6, 2022

SyntaxNode left a comment

SyntaxNode Sep 19, 2022

bsardo Sep 20, 2022

bsardo Sep 20, 2022

bsardo Sep 26, 2022

SyntaxNode Sep 26, 2022

AlexBVolcy Sep 26, 2022

SyntaxNode Sep 26, 2022

SyntaxNode Sep 26, 2022

SyntaxNode Sep 26, 2022

SyntaxNode Sep 26, 2022

SyntaxNode Sep 26, 2022

bsardo Sep 27, 2022

AlexBVolcy Sep 27, 2022 •

edited

Loading

bsardo left a comment

bsardo Oct 6, 2022

Pass Host Config Info to Bid Adapters #2370

Pass Host Config Info to Bid Adapters #2370

Conversation

AlexBVolcy commented Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy commented Sep 6, 2022 •

edited

Loading

AlexBVolcy Sep 27, 2022 •

edited

Loading