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

stored_request HTTP fetcher timeout #1527

Closed
amaurybrisou opened this issue Oct 8, 2020 · 9 comments
Closed

stored_request HTTP fetcher timeout #1527

amaurybrisou opened this issue Oct 8, 2020 · 9 comments
Labels
Intent to implement An issue describing a plan for a major feature. These are intended for community feedback PBS-Go

Comments

@amaurybrisou
Copy link

amaurybrisou commented Oct 8, 2020

Hello,

I'm currently using two different servers to host prebid-server instance and on the other server my small stored_request api.

For testing & performance purposes, I just have a php file echoing a basic AMP stored request.

But I keep having timeout issues.

this timeout seems to be defined in endpoints/openrtb2/auction.go:42
50ms then.

Is http fetcher relevant with such a tiny timeout to get the stored_request back to prebid-server ?

Hope my question makes sense,

Thanks,

Amaury Brisou

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Oct 8, 2020

Is http fetcher relevant with such a tiny timeout to get the stored_request back to prebid-server ?

Yes, at least in our experience. The data layer should be as fast as possible as to use as little time of the auction as possible. If your stored requests requests are taking longer than 50ms, you might want to look into caching options.

I'm fine if you want to make this configurable though.

@laurb9
Copy link
Contributor

laurb9 commented Oct 8, 2020

My view on this, given that PBS handles very large amounts of traffic with tiny revenue per request, is that the revenue is made up by volume and cache misses are sacrificial - assumed timed out, lost in the noise. In which case a higher timeout would help ensure at least next time the cache is populated.

In my opinion it is not worth the expense or complexity of trying to save them especially at low auction timeouts. I assume the same is the case with direct database lookups.

@amaurybrisou
Copy link
Author

I'm fine if you want to make this configurable though.

Do you want a global timeout for all fetchers or specific to http endpoint, amp http endpoint ?

@SyntaxNode
Copy link
Contributor

Do you want a global timeout for all fetchers or specific to http endpoint, amp http endpoint ?

The hardcoded value is currently global, so I think it's best to replace it with a global config value for all endpoints with a default of the hardcoded 50ms value. The underlying fetchers are pretty much the same, so I think it makes sense.

Are you volunteering to code it up? 🤞

In which case a higher timeout would help ensure at least next time the cache is populated.

Makes sense. I think each host should have the flexibility to set whatever value works best for their strategy. I'd like to leave the default at 50ms simply to avoid breaking changes.

@amaurybrisou
Copy link
Author

amaurybrisou commented Oct 9, 2020

yep I'm in asap.

Btw I wanted to create a pbs.config.default what do you think ? in another PR oc

@SyntaxNode
Copy link
Contributor

yep I'm in asap.

Fantastic!

Btw I wanted to create a pbs.config.default what do you think ? in another PR oc

Yes, please keep them separate. What's your intent behind this file? We currently hardcode the default values in the app. If you're thinking of a starting point to help hosts get started quickly.. that would be awesome, but perhaps better suited for a markdown doc.

@amaurybrisou
Copy link
Author

amaurybrisou commented Oct 9, 2020

yes easier config access and it looks to me a good habit to have such file

@amaurybrisou
Copy link
Author

@SyntaxNode I opened the PR, tell me if you want another naming or something else. #1533

@SyntaxNode SyntaxNode added Intent to implement An issue describing a plan for a major feature. These are intended for community feedback PBS-Go labels Oct 16, 2020
@SyntaxNode
Copy link
Contributor

Closed in favor of the duplicate #2362 issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Intent to implement An issue describing a plan for a major feature. These are intended for community feedback PBS-Go
Projects
None yet
Development

No branches or pull requests

3 participants