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

PBS adapter: add tmaxmax #11540

Merged
merged 10 commits into from
Jun 3, 2024
Merged

PBS adapter: add tmaxmax #11540

merged 10 commits into from
Jun 3, 2024

Conversation

jefftmahoney
Copy link
Contributor

@jefftmahoney jefftmahoney commented May 22, 2024

Type of change

  • Feature

Description of change

Update the logic in ortbConverter.js so that a tmaxmax property will now be available for s2s requests. The value of tmaxmax will be either something specified by the publisher (in setConfig() example below) or, failing that, will fall back to the value of requestBidsTimeout.

Sample setup that a publisher would do to invoke this new logic

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

Other information

The work of this PR addresses this issue

@ChrisHuie ChrisHuie changed the title ortbConverter: Tmax - Update logic for how it is determined PBjs Core ortbConverter : update logic for how tmaxmax is determined May 22, 2024
@@ -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

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:

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

passed down here:

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

and finally here:

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:

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

@dgirardi dgirardi changed the title PBjs Core ortbConverter : update logic for how tmaxmax is determined PBS adapter: add tmaxmax May 22, 2024
* Update `request()` method in ortbConverter.js to properly account for fact that tmaxmax is actually now a new available property off of the ext property.
* Update unit test to verify that when a publisher specifies a tmaxmax value in the config setup, it will be honored.
@@ -58,6 +58,7 @@ const PBS_CONVERTER = ortbConverter({
const request = buildRequest(imps, proxyBidderRequest, context);

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

Choose a reason for hiding this comment

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

After testing this I realized there's a few more steps to take. proxyBidderRequest.timeout here is undefined as it's filtered out here. However, it turns out that timeout is not the one we want:

timeout: s2sConfig.timeout,

my suggestion is to add requestBidsTimeout here:

let s2sBidRequest = {'ad_units': adUnitsS2SCopy, s2sConfig, ortb2Fragments};

and then pick it up from here (should be available under context.s2sBidRequest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I've made the suggested changes - or what I understand the suggested changes to be - with 4bb1b47. Happy to answer any questions or follow up as needed!

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 actually, I just tried running my test where I just do this

// request.ext.tmaxmax = request.ext.tmaxmax || context.s2sBidRequest.requestBidsTimeout; --> commenting out temporarily to see what happens if request.ext.tmaxmax is not available for assignment

request.ext.tmaxmax = context.s2sBidRequest.requestBidsTimeout;  

And my test (line 791 of prebidServerBidAdapter_spec.js) fails because the actual value is undefined. Which I guess means requestBidsTimeout is undefined.

I think this means that I need to do additional test-data setup to mimic requestBidsTimeout being available in a real-world scenario.

I'll dig more into this and let you know if further logic changes are required in ortbConverter.js, adapterManager.js or elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this means that I need to do additional test-data setup to mimic requestBidsTimeout being available in a real-world scenario.

Only if you add a test case that covers the fallback - which I won't object to. Currently it should pass as it only checks that the input tmaxmax has precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I can verify that it does pass in its current iteration. I'll try adding a fallback test too in a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fallback test added.

* Update ortbConverter logic so that `requestBidsTimeout` is the alternative value when `tmaxmax` is not available.
* Add a test to verify that fallback for `tmaxmax` is `requestBidsTimeout`.
@patmmccann patmmccann merged commit 524c2ad into prebid:master Jun 3, 2024
4 checks passed
DecayConstant pushed a commit to mediavine/Prebid.js that referenced this pull request Jul 18, 2024
* Update logic to determine tmax

* Update the logic in `ortbConverter.js` so that if a publisher specifies `tmaxmax` as a part of their setup (i.e. specifies it in their `setConfig()` call), `tmaxmax` is what is used to determine `tmax` for a request.

* Update tmaxmax logic and test

* Update `request()` method in ortbConverter.js to properly account for fact that tmaxmax is actually now a new available property off of the ext property.
* Update unit test to verify that when a publisher specifies a tmaxmax value in the config setup, it will be honored.

* Use requestBidsTimeout for tmaxmax alternative

* Update ortbConverter logic so that `requestBidsTimeout` is the alternative value when `tmaxmax` is not available.

* Update prebidServerBidAdapter_spec.js

* Add a test to verify that fallback for `tmaxmax` is `requestBidsTimeout`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants