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 Fetch Timeout Should be Configurable #2362

Open
GLStephen opened this issue Aug 29, 2022 · 7 comments
Open

Stored Request Fetch Timeout Should be Configurable #2362

GLStephen opened this issue Aug 29, 2022 · 7 comments
Assignees
Labels

Comments

@GLStephen
Copy link
Contributor

GLStephen commented Aug 29, 2022

The value of storedRequestTimeoutMillis is fixed in the auction.go file. However, it's entirely conceivable that HTTP requests may be slower than 50ms. With caching etc. there should be a way to allow longer timeouts here since only one hit takes the latency. In addition, in dev environments it may need to be configured separate from production.

const storedRequestTimeoutMillis = 50

I would simply submit a PR, but there seems to be a lot of "timeout" vars already in config that may be relevant.

There is this timeout for http_events, but no similar one for storedRequests.

v.SetDefault("stored_requests.http_events.timeout_ms", 0)

Any opinions on approach here? New config? Extend existing config?

We could extend in places we don't currently, such as "stored_requests.http.timeout_ms" and use it for both AMP and non-amp. Or add a global storedRequestTimeoutMillis "stored_requests.timeout_ms" that simply maps to this hardcoded value. Opinions?

@bsardo bsardo self-assigned this Aug 30, 2022
@SyntaxNode
Copy link
Contributor

This is a duplicate of #1527. I'll close the duplicate issue and keep this one open.

This sounds good to me. We can scope the feature together in this issue. Once we're in agreement, are you offering to code it?

@SyntaxNode
Copy link
Contributor

Discussed on the Prebid Server Committee meeting last week. We're considering if there should even be a stored request fetch timeout or if it should be part of the auction's tmax time. Do you have an opinion?

@GLStephen
Copy link
Contributor Author

We do intend to the dev work here. @SyntaxNode

There are two configs I can see:

  1. No cache, straight HTTP fetcher
  2. Cache + HTTP Fetcher

Why would you run 1 in production? If not running in production then it's really a dev config (primarily). Setting it very long may be valid in dev far more than an auction timeout for various reasons. In production, the timer should only effect one request every X requests. However, you want it to complete so it gets cached. So, I would think a distinct value, not part of the auction timeout makes sense. Then you have at worst a "sacrificial" request, but it completes so the next ones are fast in the 2nd (production) config.

@bretg
Copy link
Contributor

bretg commented Sep 23, 2022

Are there any open questions here or is this ready-for-dev?

@GLStephen
Copy link
Contributor Author

@bretg if it's ready can you assign it to and we'll get it done?

@GLStephen
Copy link
Contributor Author

@bretg I guess this got left in "needs design" - how can I drive this forward? I might have dropped the ball on driving design. Let me know what I can do.

@bretg bretg moved this from Needs Requirements to Ready for Dev in Prebid Server Prioritization Dec 1, 2023
@bretg bretg removed the Needs Design label Dec 1, 2023
@bretg bretg assigned GLStephen and unassigned bsardo Aug 23, 2024
@bretg
Copy link
Contributor

bretg commented Aug 23, 2024

@GLStephen - somewhere along the line this got set to ready-for-dev but never got assigned to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Ready for Dev
Development

No branches or pull requests

4 participants