-
Notifications
You must be signed in to change notification settings - Fork 512
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
[query-frontend] Set write deadline for /active_series
requests
#7553
Conversation
ef79cbe
to
bc0363e
Compare
bc0363e
to
edd4aab
Compare
/active_series
requests/active_series
requests
pkg/frontend/transport/handler.go
Outdated
@@ -53,13 +53,15 @@ type HandlerConfig struct { | |||
LogQueryRequestHeaders flagext.StringSliceCSV `yaml:"log_query_request_headers" category:"advanced"` | |||
MaxBodySize int64 `yaml:"max_body_size" category:"advanced"` | |||
QueryStatsEnabled bool `yaml:"query_stats_enabled" category:"advanced"` | |||
ResponseStreamTimeout time.Duration `yaml:"response_stream_timeout" category:"experimental"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered to name the config parameter to be specific for active series endpoints? Reason is that if in the future we add streaming response to other endpoints, I would argue a 10m default timeout for them may be very high and undesiderable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this looks applied only to write timeout, we could rename it to "active-series-write-timeout" to be more specific.
pkg/frontend/transport/handler.go
Outdated
// Set write deadline for streaming responses. | ||
var deadline time.Time | ||
if f.cfg.ResponseStreamTimeout > 0 { | ||
deadline = time.Now().Add(f.cfg.ResponseStreamTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't set a no deadline if ResponseStreamTimeout
is 0. On the contrary, the whole if isStreamingEndpoint(r)
should be skipped if ResponseStreamTimeout == 0
and then we document that if it's zero the global timeout will be used. I think it's a bad practice allowing to have no timeout (even if someone could set a very high timeout, but at least it's more clear the intention).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the behaviour to apply the longer timeout only if it's configured and otherwise default to the same timeout as the other endpoints.
{name: "deadline not exceeded for streaming endpoint", path: "/api/v1/cardinality/active_series"}, | ||
} { | ||
t.Run(tt.name, func(t *testing.T) { | ||
const serverWriteTimeout = 10 * time.Millisecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would increase it. Being very short, it could cause some flakyness on a slow CI.
@@ -158,6 +158,10 @@ func (w *GzipResponseWriter) Write(b []byte) (int, error) { | |||
return w.startPlainWrite(len(b)) | |||
} | |||
|
|||
func (w *GzipResponseWriter) Unwrap() http.ResponseWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why is this change required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment that explains why this is required.
if f.cfg.ResponseStreamTimeout > 0 { | ||
deadline = time.Now().Add(f.cfg.ResponseStreamTimeout) | ||
} | ||
err = http.NewResponseController(w).SetWriteDeadline(deadline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm we don't need to also set the read deadline, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the requests are typically small and the default read deadline should be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for addressing my feedback
What this PR does
The query-frontend uses the value supplied via
-server.http-write-timeout
(default2m
) to close connections that have not finished writing the response by the deadline. The/active_series
endpoint can generate large responses, the transmission of which can exceed that deadline. Therefore, this PR introduces a parameter to configure write timeouts for streaming responses (currently only applicable to the/active_series
endpoint).Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.