-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Relevant Digital Bid Adapter: initial release #9685
Relevant Digital Bid Adapter: initial release #9685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this new bid adapter. Please see below for some requested changes.
In addition, I have some concerns regarding the overall structure of the unit tests. They seem to be actually running an auction (unless I'm missing something), which is generally not permitted. The unit tests for bid adapters are meant to test their main spec functions (ie buildRequests
, interpretResponse
, etc) to ensure they generate the proper output assuming some test or mocked input. If my assumption of what the current tests are doing is true, can you please update the tests?
CCing @patmmccann for his thoughts on this point.
modules/relevantDigitalBidAdapter.js
Outdated
const editBid = (bid) => { // Modify bidder-codes in bid to the bidder code for this bidder | ||
const { prebid = {} } = bid.ext || {}; | ||
const { meta, targeting } = prebid; | ||
if (meta?.adaptercode) { | ||
meta.adaptercode = bidder; | ||
} | ||
if (targeting?.hb_bidder) { | ||
targeting.hb_bidder = bidder; | ||
} | ||
return bid; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this overall behavior. It does against the general rules for bid adpaters modifying values in targeting. But this is also a unique case given you're calling PBS endpoint via this adapter and the bids would normally come back as someone else instead of your own.
@patmmccann Do you have any concerns here? Or is this an okay exception to allow in their case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that it might cause some reporting-confusion/problems for publishers that already have for example, their own Pubmatic account. They would then get ad requests with hb_bidder=pubmatic
- that in fact belongs to this integration instead of their own account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, this feels useful to discuss in committee. Why not just label all the bids that come back via this adapter as bidder-code relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I guess the other option would be that we would transform the response in the Prebid Server code instead. However, we really try hard to avoid own customisations of the Prebid Server source code.
So just to clarify a bit, what's happening here is that:
- A normal Prebid Server instance is called (with stored request ids, so that the actual SSPs can be configured on the servers)
- We get the response back
interpretResponse
creates a copy of the server'sBidResponse
and modifies it so that it appears as the result of a single bidder/adapter"relevantdigital"
- to not confuse analytic adapters / 3'rd party libraries, etc.- This involves changing
hb_bidder=relevantdigital
- in this copy of the server's response. - After this, the ORTB conversion library handles the rest by creating the response object using this modified server-response.
So we're not changing the targeting in the bid's adserverTargeting
object. Instead we're just doing some preprocessing of a copy of the server's response - before feeding it to ORTB covertion library's fromORTB()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on our offline discussion; I now understand what you're doing a bit better and I think it's OK. As you say, this logic is working on the response from your backend (and not on the "real" bid targeting as I - and maybe @jsnellbaker - initially thought).
Some of it can be made simpler; instead of modifying seatbid.seat
and ext.prebid.meta.adaptercode
I would do:
const converter = ortbConverter({
// ...
overrides: {
bidResponse: {
bidderCode: false,
}
}
})
This would have the effect of turning off this piece of logic, which is what's copying over bidderCode
and adapterCode
from the PBS response; without it, they default to whatever alias was used in the adUnit (which would also address that concern). To make it configurable:
const converter = ortbConverter({
// ...
overrides: {
bidResponse: {
bidderCode(orig, bidResponse, bid, context) {
if (/* should bidder be the "true" bidder? */) {
orig(bidResponse, bid, context); // run the "vanilla" logic (set bidder and adapter code as returned by PBS)
}
}
}
}
})
You could do something similar with extPrebidTargeting
or extPrebidMeta
but it may not be much simpler unless it makes sense to turn them off entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/relevantDigitalBidAdapter.js
Outdated
|
||
/** Settings ber bidder-code. checkParams === true means that it can optionally be set in bid-params */ | ||
const FIELDS = [ | ||
{ name: 'pbsHost', checkParams: true, required: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From your comment in the description, I understand there's a current need to parametrize the domain of the endpoint. But is that only a temporary plan and you will eventually be able to establish a set domain in your adapter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the foreseeable future we'll need to continue use different domains. The "problem" is that our Prebid Server instances are already up and running - using different domain names.
The use-case for this adapter is that some existing network-customers should be able to let publishers use "just another" Prebid adapter instead of forcing them to run our normal js-wrapper on their sites.
Seems I had missed the no-auction-rule for test-cases, so I've now rewritten the tests. My concern was that as this adapter is mostly a "wrapper" around the ORTB conversion library, any future change/addition to the bidderRequest-object then used by the conversion-library might break these tests. Therefore I was a bit hesitant to mock that object. But hopefully it won't be a problem, so it's changed now. |
…of the client-side code in responses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the updated logic as per the most recent updates. It appears to work as was described in the thread (with the useSourceBidderCode
param).
LGTM
* Relevant Digital Bid Adapter * More tests + comments * Use the recommended onBidWon callback + live-placements in .md file * Remove unused imports + adjust example-parameters in .md file * Renamed files + rewritten test-cases * Added documentation for 'pbsBufferMs' setting * Added 'useSourceBidderCode' setting to use S2S bidder's code instead of the client-side code in responses
Type of change
Description of change
This is a bid adapter for our Relevant Yield platform. The endpoint will be a Prebid Server instance using stored requests whose ids will be supplied via
params.placementId
.Test parameters for validating bids:
Recommended way is to use
setConfig()
forpbsHost
andaccountId
instead, see examples in the .md file.Other information
I understand that the dynamic host-name setting might not be fully compatible with the module rule saying "Endpoint domain names cannot be fully variable". However, we haven't really any way around that at the moment as we're already using different hosts (just not via this new type of integration). Therefore we hope for some understanding.
PR for the bid params in the documentation repository: prebid/prebid.github.io#4426