-
Notifications
You must be signed in to change notification settings - Fork 168
Propagate context from query requests #1610
Conversation
e9ee3dd
to
dcb9ffe
Compare
dcb9ffe
to
3961e69
Compare
3961e69
to
13f6abf
Compare
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.
Overall, this looks good. I'd like to see some tests about what happens if the context expires but am not 100% positive it's practical to do without creating flaky tests. It's worth thinking about and considering whether such tests are possible though,
Each http.Request comes with its own context, in the case the client connection closes this context is cancelled. Propagating the request context prevents the request handler from doing extra work in situations when the client is not going to be able to receive the response. For supporting context propagation, the same approach from the Prometheus codebase was followed. Every `Queryable` method that returns a `Querier` accepts a context, it's the `Querier` responsibility to propagate it downstream. - Prometheus `Queryable` interface definition: https://github.com/prometheus/prometheus/blob/15fa34936b6f1febe896ecee0f49889a56347762/storage/interface.go#L79-L84 - Prometheus `Querier` propagating the ctx in the `Select` method: https://github.com/prometheus/prometheus/blob/15fa34936b6f1febe896ecee0f49889a56347762/storage/remote/read.go#L170 Even though endpoints like `/api/v1/query` and `/api/v1/query_range` were already propagating the context to downstream functions up to the `SamplesQuerier` to handle the `timeout` parameter of the request, the `Querier` was not using the given context in the calls to the `PgxConn` methods, it was using `context.Background()` instead. An important consideration to keep in mind: cancelling the context on a request for a running query will abort the connection to the database but not the query. To cancel the query we might have to use something like `statement_timeout` as proposed by #1232 .
13f6abf
to
36cc120
Compare
I don't think it's practical either. A cancelled context will be returned as an error. As I see it, the error path should be handled and tested independent of what type of error is returned during the lifecycle of a request, unless there's specific code executed depending on the type of error that's returned. With regards to these changes, the tests could be asserting that the same ctx that's given by the request is the same that's ultimately passed to the pgx methods. Given the amount of things in between, I'd say this tests won't be easy to set up and won't provide much value. I think we are ok relying on the contract that we are propagating the context and that pgx and the http library honor that contract. In the case of end 2 end the problem would be that depending on when the context is cancelled we would be testing different parts of the code. If the context is cancelled immediately we would only test the first check for the cancelled context, if the context is cancelled randomly then we won't be really sure what to assert. Of course, I'm open to ideas, because I don't have any good one for this. |
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
Description
Propagate context from query requests
Each http.Request comes with its own context, in the case the client
connection closes this context is cancelled.
Propagating the request context prevents the request handler from doing
extra work in situations when the client is not going to be able to
receive the response.
For supporting context propagation, the same approach from the
Prometheus codebase was followed. Every
Queryable
method that returnsa
Querier
accepts a context, it's theQuerier
responsibility topropagate it downstream.
Prometheus
Queryable
interface definition:https://github.com/prometheus/prometheus/blob/15fa34936b6f1febe896ecee0f49889a56347762/storage/interface.go#L79-L84
Prometheus
Querier
propagating the ctx in theSelect
method:https://github.com/prometheus/prometheus/blob/15fa34936b6f1febe896ecee0f49889a56347762/storage/remote/read.go#L170
Even though endpoints like
/api/v1/query
and/api/v1/query_range
already propagated the context to downstream functions to the
SamplesQuerier
to handle thetimeout
parameter of the request, theQuerier
was not using the given context in the calls to thePgxConn
methods, it was using
context.Background()
instead.An important consideration to keep in mind: cancelling the
context on a request for a running query will abort the connection to the
database but not the query. To cancel the query we might have to use
something like
statement_timeout
as proposed by#1232 .
closes #1205
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: