Set storedRequestTimeoutMillis as cutomizable config variable #1533
Conversation
…requests.http.fetcher_timeout
| @@ -863,6 +863,7 @@ func SetupViper(v *viper.Viper, filename string) { | |||
| v.SetDefault("stored_requests.postgres.poll_for_updates.amp_query", "") | |||
| v.SetDefault("stored_requests.http.endpoint", "") | |||
| v.SetDefault("stored_requests.http.amp_endpoint", "") | |||
| v.SetDefault("stored_requests.http.fetcher_timeout", 50) | |||
There was a problem hiding this comment.
This isn't the correct place to configure the timeout. This is an endpoint timeout for calling whatever stored request fetcher is in use rather than a configuration of the http fetcher. I recommend something along the lines of stored_request_timeout_ms at the root.
IMHO, this follows the pattern of amp_timeout_adjustment_ms and video_stored_request_required.
| @@ -226,7 +224,7 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request) (req *openrtb. | |||
| } | |||
| } | |||
|
|
|||
| timeout := parseTimeout(requestJson, time.Duration(storedRequestTimeoutMillis)*time.Millisecond) | |||
| timeout := parseTimeout(requestJson, time.Duration((*deps.cfg).StoredRequests.HTTP.FetcherTimeout)*time.Millisecond) | |||
There was a problem hiding this comment.
There's no need to dereference a copy of the config. You could just do deps.cfg.StoredRequestTimeoutMS.
Sure I can do that but where ? on the
Thanks |
At the individual endpoint level. The change you're making doesn't change the fetcher code, it's the call site in the endpoints when calling into the fetcher code. Don't worry if the code is too hard to reach with a unit test. I just think it's worth a check. The change is straight forward enough that major rework of the endpoint isn't necessary to add a test. |
|
Ok these are various things to to, I'll do it asap |
|
@amaurybrisou Friendly reminder about this PR. |
|
Yep I'm sorry, I don't have the time to code all the tests and it has quite a lot. I will asap hopefully |
No worries. Just want to keep it on your radar. I didn't mean to overwhelm you with the ask for tests. If it's simple to do, then great. If it will be too much work to find a way to test, then I'll consider that beyond the scope of this PR as there are no existing tests covering this value. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@amaurybrisou maybe you could please take a look at this again, or I can submit new PR? I mean having 50ms for external endpoint of stored requests, which can be like any IAAS or database-as-a-service out is a bit strange don't you think? Also we have millions of configs in there.. 50ms is a bit too harsh. |
|
@holms This PR was automatically closed by our stalebot when the original author stopped responding. We have no issue with adding this feature if you'd like to take over development and open a new PR. |
Set storedRequestTimeoutMillis as global config variable, allowing http fetchers timeout customization through stored_requests.http.fetcher_timeout config.
#1527