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

give s2s request the same amount of time from the client side #2790

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

mkendall07
Copy link
Member

Type of change

  • Bugfix

Description of change

The s2s timeout value was not getting the full requestBids timeout value on the client side. This was causing the request to abort on the client side if you shortened the s2s timeouts. Example, set s2s timeout to 10ms:

pbjs.setConfig({
      "s2sConfig": {
          defaultVendor : 'appnexus',
          accountId: '<id>',
          enabled: true,
          bidders: ['appnexus'],
          timeout: 10,
      }
  });

To repro.

Other information

May contribute to prebid/prebid-server#592

@mkendall07 mkendall07 changed the title give s2s request the same amount of time from the client side, ie do … give s2s request the same amount of time from the client side Jun 28, 2018
@mercuryyy
Copy link
Contributor

So after the fix can we set s2s timeout to the same value as client bidders timeout or do we still need to leave 300-400ms delay between bidder timeout and s2s timeout ?

Maybe it would be best to disable the s2sconfig timeout value and have prebid automatically set it according the bidderTimeout value?

@dbemiller
Copy link
Contributor

do we still need to leave 300-400ms delay between bidder timeout and s2s timeout ?

You should still leave the delay. It's motivated by network latency & time prebid.js may need to unpack the PBS response and get it into the auction

Maybe it would be best to disable the s2sconfig timeout value and have prebid automatically set it according the bidderTimeout value?

This is a great idea ^^

@mercuryyy
Copy link
Contributor

@dbemiller @mkendall07

Found another problem im not sure if it was fixed in the recent pulls i just saw.

If new s2s openrtb2 timeouts it will cause the entire action (client side) to never fire the action so even if there are bids in client side adapter action will never run if s2s timeouts.

This happens regardless of what timeout value you set s2s config.

@mike-chowla mike-chowla requested a review from snapwich July 3, 2018 18:29
@mike-chowla mike-chowla added the needs 2nd review Core module updates require two approvals from the core team label Jul 3, 2018
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@snapwich snapwich merged commit 8995c80 into master Jul 9, 2018
@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Jul 10, 2018

In testing the point made by @mercuryyy in this branch, it seems like the bids for the other client-side adapters would still be processed and the winning ad does show. However, it also seems like the auction doesn't complete in the normal sense. From the console logs, it stops right after it made all the requests to all the bidders and emitted the various bidRequested events.

This is a separate matter from the original issue per discussion with @mkendall07 and we'll create a new issue to address this behavior more ideally.

@dbemiller dbemiller deleted the bugfix/s2s_timeout_use_client_roundtrip branch July 11, 2018 13:40
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
florevallatmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Sep 6, 2018
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
…#2790)

* give s2s request the same amount of time from the client side, ie do not abort early

* missed auctionjs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants