Get fetch timeout from db config #1342
Conversation
benraskin92
changed the title
src/query/api/v1/httpd/handler.go
Outdated
| @@ -131,8 +132,12 @@ func (h *Handler) RegisterRoutes() error { | |||
| ).Methods(openapi.HTTPMethod) | |||
| h.router.PathPrefix(openapi.StaticURLPrefix).Handler(logged(openapi.StaticHandler())) | |||
|
|
|||
| timeoutOpts := &prometheus.TimeoutOpts{ | |||
| FetchTimeout: h.embeddedDbCfg.Client.FetchTimeout, | |||
There was a problem hiding this comment.
@r and @richardartoul are we guaranteed that embeddedDbCfg.Client != nil here?
There was a problem hiding this comment.
Rather than having this, have the handler take in a TimeoutOpts in NewHandler and set it on the Handler stuct
There was a problem hiding this comment.
I don't think its guaranteed to be set
There was a problem hiding this comment.
Okay maybe I'll just set it here then if it's nil
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
=========================================
- Coverage 70.7% 60.2% -10.6%
=========================================
Files 822 810 -12
Lines 70160 68630 -1530
=========================================
- Hits 49643 41321 -8322
- Misses 17293 24309 +7016
+ Partials 3224 3000 -224
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
========================================
- Coverage 70.7% 70.7% -0.1%
========================================
Files 822 822
Lines 70192 70204 +12
========================================
- Hits 49656 49637 -19
- Misses 17309 17332 +23
- Partials 3227 3235 +8
Continue to review full report at Codecov.
|
robskillington
changed the title
| require.NoError(t, err) | ||
| h.embeddedDbCfg = &dbconfig.DBConfiguration{Client: client.Configuration{FetchTimeout: 15 * time.Second}} | ||
|
|
||
| return h, nil |
There was a problem hiding this comment.
No need to return nil here, instead change the return to just be *Handler
| @@ -46,6 +47,10 @@ import ( | |||
|
|
|||
| var ( | |||
| promReadTestMetrics = newPromReadMetrics(tally.NewTestScope("", nil)) | |||
|
|
|||
| timeoutOpts = &prometheus.TimeoutOpts{ | |||
There was a problem hiding this comment.
Add a test to check that the timeout is being honoured?
Fix test Fix tests Address comments Fix test
benraskin92
force-pushed
the
braskin/timeout_config
branch
from
3f5aa0e to
b59f733
Compare
| defaultTimeout = time.Second * 15 | ||
| maxTimeout = 5 * time.Minute | ||
| // DefaultTimeout is the default timeout for fetch requests | ||
| DefaultTimeout = 15 * time.Second |
There was a problem hiding this comment.
It feels a bit weird to export this from here; I think it would read better if you had this hardcoded backup value in src/query/api/v1/httpd/handler.go and keep it unexported.
Then in common_test hardcode the 15 seconds as the test value (might be better to up to 30 in light of user comments on gitter?)
There was a problem hiding this comment.
Yeah I went back and forth on this. I'll put in handler.go
| req, _ := http.NewRequest("POST", PromReadURL, test.GeneratePromReadBody(t)) | ||
| dur, err := prometheus.ParseRequestTimeout(req, promRead.timeoutOpts.FetchTimeout) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 2*time.Minute, dur) |
There was a problem hiding this comment.
Nice!
Can you add a test with a default, and an invalid timeout value?
There was a problem hiding this comment.
We have these tests in the native package. Not sure we need to duplicate.
| @@ -64,17 +64,17 @@ func TestTimeoutParse(t *testing.T) { | |||
| req, _ := http.NewRequest("POST", "dummy", nil) | |||
| req.Header.Add("timeout", "1ms") | |||
|
|
|||
| timeout, err := ParseRequestTimeout(req) | |||
| timeout, err := ParseRequestTimeout(req, time.Second) | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, timeout, time.Millisecond) | |||
There was a problem hiding this comment.
Oops never mind github hid the top bit
| if embeddedDbCfg == nil { | ||
| timeoutOpts.FetchTimeout = defaultTimeout | ||
| } else { | ||
| if embeddedDbCfg.Client.FetchTimeout < 0 { |
There was a problem hiding this comment.
Quick test to check this?
No description provided.