NETOBSERV-1102: fine-tune http servers #428
Conversation
jotak
added
no-qe
|
@jotak: This pull request references NETOBSERV-1102 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@jotak: This pull request references NETOBSERV-1102 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 58.09% 57.66% -0.44%
==========================================
Files 169 171 +2
Lines 7985 8069 +84
Branches 990 975 -15
==========================================
+ Hits 4639 4653 +14
- Misses 3067 3139 +72
+ Partials 279 277 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| Handler: router, | ||
| Addr: fmt.Sprintf(":%d", cfg.Port), | ||
| TLSConfig: tlsConfig, | ||
| ReadTimeout: 30 * time.Second, |
There was a problem hiding this comment.
Is this affecting loki metrics queries ?
It was 30s and you are moving it to default = 10s here. I think some queries will take longer time than this
There was a problem hiding this comment.
As far as I understand the read timeout only impacts when the server reads the content of the HTTP requests coming from the frontend, so in general a short timeout is ok, unless there's a lot of data to read such as in case of file uploads, which I think doesn't happen for us (?). So - if I'm correct - long loki queries are only impacted by the Write timeout, which can make them fail, as this code is invoked in the write path of the request processing.
In this blog there's a schema showing what each timeout covers: https://bruinsslot.jp/post/go-secure-webserver/index.html
I took it as a reference, they use a default of 5s for read timeout. Here I wanted a compromise by setting 10s default.
I don't have strong opinion against setting it back to 30s though, if we think 10s isn't enough, but my understanding of how timeouts work + our usage make me thing this should be good
There was a problem hiding this comment.
Let's stick with the default and we'll increase it if needed. Thanks for the details !
|
/appove |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Provide defaults settings for http servers, overridable by callers
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.