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.

Sign up for GitHub

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

Jump to bottom

PBS adapter: add tmaxmax #11540

Merged
merged 10 commits into from
Jun 3, 2024
Merged

PBS adapter: add tmaxmax #11540

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1659209
Merge pull request #1586 from prebid/master
jefftmahoney Jan 9, 2024
a49852a
Merge branch 'master' of https://github.com/sharethrough/Prebid.js-in…
jefftmahoney Jan 19, 2024
bc2e207
Merge branch 'master' of https://github.com/sharethrough/Prebid.js-in…
jefftmahoney Feb 2, 2024
25e0e24
Merge branch 'master' of https://github.com/sharethrough/Prebid.js-in…
jefftmahoney Mar 19, 2024
c5495ba
Merge branch 'master' of https://github.com/sharethrough/Prebid.js-in…
jefftmahoney May 9, 2024
18c8328
Merge branch 'master' of https://github.com/sharethrough/Prebid.js-in…
jefftmahoney May 20, 2024
6a2a7cd
Update logic to determine tmax
jefftmahoney May 22, 2024
b3b3ca4
Update tmaxmax logic and test
jefftmahoney May 23, 2024
4bb1b47
Use requestBidsTimeout for tmaxmax alternative
jefftmahoney May 29, 2024
f20d821
Update prebidServerBidAdapter_spec.js
jefftmahoney May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/prebidServerBidAdapter/ortbConverter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const PBS_CONVERTER = ortbConverter({
let {s2sBidRequest, requestedBidders, eidPermissions} = context;
const request = buildRequest(imps, proxyBidderRequest, context);

request.tmax = s2sBidRequest.s2sConfig.timeout;
request.tmax = s2sBidRequest.s2sConfig.ortb2?.ext?.tmaxmax || s2sBidRequest.s2sConfig.timeout;
Copy link
Collaborator

@dgirardi dgirardi May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my reading of the issue:

This is great, but we have a module that would like to know the actual client-side timeout -- the timeout passed to pbjs.requestBids.

The ortbConverter library should check for the existence of ext.tmaxmax and if not there, set it to the timeout value passed to requestBids()

is that this should be a new field, request.ext.tmaxmax, which by default should be the client timeout, optionally overridable with ortb2.ext.tmaxmax

which I think in code translates to

Suggested change
request.tmax = s2sBidRequest.s2sConfig.ortb2?.ext?.tmaxmax || s2sBidRequest.s2sConfig.timeout;
request.tmax = s2sBidRequest.s2sConfig.timeout;
request.ext.tmaxmax = request.ext.tmaxmax ?? proxyBidderRequest.timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm ... yes, I think I agree with you @dgirardi.

Belated question: do we think that tmaxmax is something that a SSP endpoint (the place a bid request will get POST-ed to) needs to worry about? Or is this just to help the server-side make a smart decision about tmax? I'm assuming since tmaxmax is not part of oRTB the answer is "no", but thought I'd ask.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is of interest to prebid server specifically because it has a separate timeout configuration that doesn't necessarily match the client timeout. Client adapters typically already have tmax set to the client timeout, and if not they can choose to do as they wish, so I don't think it makes sense to generalize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgirardi I'm having trouble writing a test to verify that this works.

Where would ortbConverter.js look to find out if a publisher had made this call:

pbjs.setConfig({
   ortb2: {
      ext: {
         tmaxmax: 888
      }
   }
});

Would it be in proxyBidderRequest? That object is undefined right now in the test I'm running, but if that is where a publisher-specified tmaxmax value would go I could mock one up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roughly, ortb2 comes in as s2sBidRequest.ortb2Fragments.global here:

Prebid.js/modules/prebidServerBidAdapter/index.js

Line 454 in e074512

baseAdapter.callBids = function(s2sBidRequest, bidRequests, addBidResponse, done, ajax) {

passed down here:

Prebid.js/modules/prebidServerBidAdapter/ortbConverter.js

Line 262 in e074512

export function buildPBSRequest(s2sBidRequest, bidderRequests, adUnits, requestedBidders, eidPermissions) {

and finally here:

Prebid.js/modules/prebidServerBidAdapter/ortbConverter.js

Line 172 in e074512

mergeDeep(ortbRequest, context.s2sBidRequest.ortb2Fragments?.global);

which is how, in the code at the top of this thread, it ends up filling request.ext.tmaxmax after you run buildRequest.

"roughly" because there's more than one way to configure ortb2 - and the PBS adapter is peculiar in that it also has to deal with bidder-specific configuration separately from global configuration (hence the .global)

in practice for testing that means manipulating ortb2Fragments.global in the first argument to callBids - here's an example:

Prebid.js/test/spec/modules/prebidServerBidAdapter_spec.js

Lines 2614 to 2621 in e074512

const ortb2Fragments = {
global: {site: commonSite, user: commonUser, badv, bcat},
bidder: Object.fromEntries(allowedBidders.map(bidder => [bidder, {site, user, bcat, badv}]))
};
// adapter.callBids({ ...REQUEST, s2sConfig: cfg }, BID_REQUESTS, addBidResponse, done, ajax);
adapter.callBids(addFpdEnrichmentsToS2SRequest({...s2sBidRequest, ortb2Fragments}, bidRequests, cfg), bidRequests, addBidResponse, done, ajax);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great - thank you so much! I'll try some of this out later this afternoon and let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with commit b3b3ca4


[request.app, request.dooh, request.site].forEach(section => {
if (section && !section.publisher?.id) {
Expand Down
13 changes: 13 additions & 0 deletions test/spec/modules/prebidServerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,19 @@ describe('S2S Adapter', function () {
expect(req.tmax).to.eql(123);
});

it('should set tmax correctly if tmaxmax present in configuration', () => {
const tmaxCfg = {
ext: {
tmaxmax: 4242
}
}
const cfg = { ...CONFIG, ortb2: tmaxCfg };
config.setConfig({ s2sConfig: cfg });
adapter.callBids({ ...REQUEST, s2sConfig: cfg }, BID_REQUESTS, addBidResponse, done, ajax);
const req = JSON.parse(server.requests[0].requestBody);
expect(req.tmax).to.eql(4242);
});

it('should block request if config did not define p1Consent URL in endpoint object config', function () {
let badConfig = utils.deepClone(CONFIG);
badConfig.endpoint = { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' };
Expand Down