Configurable Stored Request Fetch Timeout #3614
Conversation
config/config_test.go
Outdated
| @@ -159,6 +159,7 @@ func TestDefaults(t *testing.T) { | |||
| cmpBools(t, "adapter_connections_metrics", true, cfg.Metrics.Disabled.AdapterConnectionMetrics) | |||
| cmpBools(t, "adapter_gdpr_request_blocked", false, cfg.Metrics.Disabled.AdapterGDPRRequestBlocked) | |||
| cmpStrings(t, "certificates_file", "", cfg.PemCertsFile) | |||
| cmpInts(t, "stored_requests.timeout_ms", 50, cfg.StoredRequests.Timeout) | |||
There was a problem hiding this comment.
Can you update TestDefaults and the corresponding test config yaml so we can verify that this new field can be set to a value other than the default 50?
config/stored_requests.go
Outdated
| @@ -71,6 +71,8 @@ type StoredRequests struct { | |||
| // HTTPEvents configures an instance of stored_requests/events/http/http.go. | |||
| // If non-nil, the server will use those endpoints to populate and update the cache. | |||
| HTTPEvents HTTPEventsConfig `mapstructure:"http_events"` | |||
| // Timeout defines the number of milliseconds before a timeout occurs with stored requests fetch | |||
| Timeout int `mapstructure:"timeout_ms"` | |||
There was a problem hiding this comment.
The point was raised in one of the closed related issues (#1533 (comment)) that perhaps this should be a top level config. Moving it down a level as you have does increase flexibility though as it allows for different timeouts for each data type that may be fetched (requests, responses, accounts, categories, etc). Let's discuss with the group.
There was a problem hiding this comment.
Sounds good, we can bring this up tomorrow!
There was a problem hiding this comment.
Discussed Today: I'm investigating the tradeoff between leaving the timeout config at this level and updating the other various config variable usages that are also of type StoredRequests to utilize the timeout appropriately, or if it would be easier to just have this variable at the root level.
bsardo
changed the title
Implements #2362