add a metric to track request timeouts #2419
Conversation
This comment has been minimized.
This comment has been minimized.
apparently there's an issue with the downcast from error here. I think we should move towards a timeout layer that returns a Ok(response) instead of Err(Elapsed)
this is a bit complicated because the timeout future only knows about very limited generic parameters, while we actually want to: - generate different responses if we're in supergraph or subgraph - return a response containing the request context
|
🤔 if timeout returns a HTTP response, then that breaks the retry layer, which will only retry on |
Geal
requested review from
a team,
SimonSapin and
abernix
and removed request for
a team
| // we do the entire supergraph rebuilding instead of using `from_supergraph_mock_callback_and_configuration` | ||
| // because we need the plugins to apply on the supergraph |
There was a problem hiding this comment.
Could you say more about what this means? The amount of duplicated boilerplate code feels unfortunate
There was a problem hiding this comment.
from_supergraph_mock_callback_and_configuration creates a mock supergraph without applying plugins on it, because it is meant as a foundation to test the router service. Here I want to test, at the router service response, the result of executing a plugin between the router service and supergraph service. That boiler plate could be extracted in another function if we see another case where we'd need it
This adds a counter for timeouts. It will generate separate metrics for client request timeouts and subgraph requests timeouts.
Checklist
Complete the checklist (and note appropriate exceptions) before a final PR is raised.