logging(apis): better logging for api failures (#56432)

This PR ensures we pass in the URL an the integration id into failure
log messages

Author: Stephen Cefali

src/sentry/integrations/slack/client.py

@@ -84,6 +84,7 @@ def track_response_data(
         span: Span | None = None,
         error: Optional[str] = None,
         resp: Optional[Response] = None,
+        extra: Optional[Mapping[str, str]] = None,
     ) -> None:
         # if no span was passed, create a dummy to which to add data to avoid having to wrap every
         # span call in `if span`
@@ -121,6 +122,7 @@ def track_response_data(
         )
 
         extra = {
+            **(extra or {}),
             self.integration_type: self.name,
             "status_string": str(code),
             "error": str(error)[:256] if error else None,

src/sentry/shared_integrations/client/base.py

@@ -243,6 +243,11 @@ def _request(
             if span and existing_transaction:
                 span.set_data("existing_transaction", existing_transaction)
 
+            extra = {"url": full_url}
+            # It shouldn't be possible for integration_type to be null.
+            if self.integration_type:
+                extra[self.integration_type] = self.name
+
             try:
                 with build_session() as session:
                     finalized_request = self.finalize_request(_prepared_request)
@@ -266,30 +271,27 @@ def _request(
                         return resp
                     resp.raise_for_status()
             except RestrictedIPAddress as e:
-                self.track_response_data("restricted_ip_address", span, e)
+                self.track_response_data("restricted_ip_address", span, e, extra=extra)
                 self.record_error(e)
                 raise ApiHostError.from_exception(e) from e
             except ConnectionError as e:
-                self.track_response_data("connection_error", span, e)
+                self.track_response_data("connection_error", span, e, extra=extra)
                 self.record_error(e)
                 raise ApiHostError.from_exception(e) from e
             except Timeout as e:
-                self.track_response_data("timeout", span, e)
+                self.track_response_data("timeout", span, e, extra=extra)
                 self.record_error(e)
                 raise ApiTimeoutError.from_exception(e) from e
             except HTTPError as e:
                 error_resp = e.response
                 if error_resp is None:
-                    self.track_response_data("unknown", span, e)
+                    self.track_response_data("unknown", span, e, extra=extra)
 
-                    # It shouldn't be possible for integration_type to be null.
-                    extra = {"url": full_url}
-                    if self.integration_type:
-                        extra[self.integration_type] = self.name
                     self.logger.exception("request.error", extra=extra)
                     self.record_error(e)
                     raise ApiError("Internal Error", url=full_url) from e
-                self.track_response_data(error_resp.status_code, span, e)
+
+                self.track_response_data(error_resp.status_code, span, e, extra=extra)
                 self.record_error(e)
                 raise ApiError.from_response(error_resp, url=full_url) from e
 
@@ -301,19 +303,19 @@ def _request(
                 # which is a ChunkedEncodingError caused by a ProtocolError caused by a ConnectionResetError.
                 # Rather than worrying about what the other layers might be, we just stringify to detect this.
                 if "ConnectionResetError" in str(e):
-                    self.track_response_data("connection_reset_error", span, e)
+                    self.track_response_data("connection_reset_error", span, e, extra=extra)
                     self.record_error(e)
                     raise ApiConnectionResetError("Connection reset by peer", url=full_url) from e
                 # The same thing can happen with an InvalidChunkLength exception, which is a subclass of HTTPError
                 if "InvalidChunkLength" in str(e):
-                    self.track_response_data("invalid_chunk_length", span, e)
+                    self.track_response_data("invalid_chunk_length", span, e, extra=extra)
                     self.record_error(e)
                     raise ApiError("Connection broken: invalid chunk length", url=full_url) from e
 
                 # If it's not something we recognize, let the caller deal with it
                 raise e
 
-            self.track_response_data(resp.status_code, span, None, resp)
+            self.track_response_data(resp.status_code, span, None, resp, extra=extra)
 
             self.record_response(resp)
 

src/sentry/shared_integrations/track_response.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import logging
+from typing import Mapping, Optional
 
 from django.utils.functional import cached_property
 from rest_framework.response import Response
@@ -41,6 +42,7 @@ def track_response_data(
         span: Span | None = None,
         error: Exception | None = None,
         resp: Response | None = None,
+        extra: Optional[Mapping[str, str]] = None,
     ) -> None:
         # if no span was passed, create a dummy to which to add data to avoid having to wrap every
         # span call in `if span`
@@ -57,12 +59,13 @@ def track_response_data(
         except ValueError:
             span.set_status(str(code))
 
-        extra = {
+        log_params = {
+            **(extra or {}),
             "status_string": str(code),
             "error": str(error)[:256] if error else None,
         }
         if self.integration_type:
-            extra[self.integration_type] = self.name
+            log_params[self.integration_type] = self.name
 
-        extra.update(getattr(self, "logging_context", None) or {})
-        self.logger.info(f"{self.integration_type}.http_response", extra=extra)
+        log_params.update(getattr(self, "logging_context", None) or {})
+        self.logger.info(f"{self.integration_type}.http_response", extra=log_params)