fix(forwarding): Change HTTP response for upstream timeouts from 502 to 504

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 - Emit the `quantity` field for outcomes of events. This field describes the total size in bytes for attachments or the event count for all other categories. A separate outcome is emitted for attachments in a rejected envelope, if any, in addition to the event outcome. ([#942](https://github.com/getsentry/relay/pull/942))
 - Add experimental metrics ingestion without bucketing or pre-aggregation. ([#948](https://github.com/getsentry/relay/pull/948))
+- Change HTTP response for upstream timeouts from 502 to 504. ([#859](https://github.com/getsentry/relay/pull/859))
 
 ## 21.3.0
 

relay-server/src/actors/upstream.rs

@@ -45,18 +45,16 @@ use crate::http::{HttpError, Request, RequestBuilder, Response};
 use crate::metrics::{RelayHistograms, RelayTimers};
 use crate::utils::{self, ApiErrorResponse, IntoTracked, RelayErrorAction, TrackedFutureFinished};
 
-#[derive(Fail, Debug)]
-#[fail(display = "could not send request using reqwest")]
-pub struct UpstreamSendRequestError(#[cause] reqwest::Error);
-
 #[derive(Fail, Debug)]
 pub enum UpstreamRequestError {
     #[fail(display = "attempted to send upstream request without credentials configured")]
     NoCredentials,
 
+    /// As opposed to HTTP variant this contains all network errors.
     #[fail(display = "could not send request to upstream")]
-    SendFailed(#[cause] UpstreamSendRequestError),
+    SendFailed(#[cause] reqwest::Error),
 
+    /// Likely a bad HTTP status code or unparseable response.
     #[fail(display = "could not send request")]
     Http(#[cause] HttpError),
 
@@ -575,7 +573,6 @@ impl UpstreamRelay {
             let res = client
                 .execute(client_request.0)
                 .await
-                .map_err(UpstreamSendRequestError)
                 .map_err(UpstreamRequestError::SendFailed);
             tx.send(res)
         });

relay-server/src/endpoints/forward.rs

@@ -76,6 +76,13 @@ impl ResponseError for ForwardedUpstreamRequestError {
                 HttpError::ActixPayload(e) => e.error_response(),
                 HttpError::ActixJson(e) => e.error_response(),
             },
+            UpstreamRequestError::SendFailed(e) => {
+                if e.is_timeout() {
+                    HttpResponse::GatewayTimeout().finish()
+                } else {
+                    HttpResponse::BadGateway().finish()
+                }
+            }
             e => {
                 // should all be unreachable
                 relay_log::error!(

tests/integration/test_forwarding.py

@@ -1,5 +1,6 @@
 import errno
 import gzip
+import time
 
 import pytest
 import requests
@@ -102,3 +103,18 @@ def dummy_upload(**opts):
             raise
     else:
         assert response.status_code == 413
+
+
+def test_timeouts(mini_sentry, relay):
+    @mini_sentry.app.route("/api/test/timeout")
+    def hi():
+        time.sleep(60)
+
+    try:
+        relay = relay(mini_sentry)
+
+        response = relay.get("/api/test/timeout")
+        assert response.status_code == 504
+
+    finally:
+        mini_sentry.test_failures.clear()