-
Notifications
You must be signed in to change notification settings - Fork 78
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
Structured logging for DSR requests #4594
Changes from all commits
7c76eaa
6406671
9cf8803
f2f0c6c
cfdc47d
448a9ef
21398a9
acaa316
8dcda3e
28ca13c
4a67a4f
0129d08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,10 @@ | |
RateLimiterPeriod, | ||
RateLimiterRequest, | ||
) | ||
from fides.api.util.logger_context_utils import ( | ||
connection_exception_details, | ||
request_details, | ||
) | ||
from fides.api.util.saas_util import deny_unsafe_hosts | ||
from fides.config import CONFIG | ||
|
||
|
@@ -100,7 +104,7 @@ def retry_send( # type: ignore | |
def decorator(func: Callable) -> Callable: | ||
@wraps(func) | ||
def result(*args: Any, **kwargs: Any) -> Response: | ||
self = args[0] | ||
self: AuthenticatedClient = args[0] | ||
last_exception: Optional[Union[BaseException, Exception]] = None | ||
|
||
for attempt in range(retry_count + 1): | ||
|
@@ -129,6 +133,9 @@ def result(*args: Any, **kwargs: Any) -> Response: | |
last_exception = ConnectionException( | ||
f"Operational Error connecting to '{self.configuration.key}'{dev_mode_log}" | ||
) | ||
logger.bind( | ||
**connection_exception_details(exc, self.uri) | ||
).error("Connector request failed.") | ||
# requests library can raise ConnectionError, Timeout or TooManyRedirects | ||
# we will not retry these as they don't usually point to intermittent issues | ||
break | ||
|
@@ -214,22 +221,28 @@ def send( | |
|
||
response = self.session.send(prepared_request) | ||
|
||
log_request_and_response_for_debugging( | ||
prepared_request, response | ||
) # Dev mode only | ||
ignore_error = self._should_ignore_error( | ||
status_code=response.status_code, errors_to_ignore=ignore_errors | ||
) | ||
context_logger = logger.bind( | ||
**request_details(prepared_request, response, ignore_error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. at some point, it'd be nice if we could figure out a not-too-crazy way to add/override third party classes (e.g. anyway, not something to worry about in this initial iteration, just thought i'd note it! |
||
) | ||
|
||
if response.ok: | ||
context_logger.info("Connector request successful.") | ||
return response | ||
|
||
if ignore_error: | ||
context_logger.info( | ||
"Connector request successful. Ignoring errors on response with status code {} as configured.", | ||
response.status_code, | ||
) | ||
return response | ||
|
||
if not response.ok: | ||
if self._should_ignore_error( | ||
status_code=response.status_code, | ||
errors_to_ignore=ignore_errors, | ||
): | ||
logger.info( | ||
"Ignoring errors on response with status code {} as configured.", | ||
response.status_code, | ||
) | ||
return response | ||
raise RequestFailureResponseException(response=response) | ||
return response | ||
context_logger.error( | ||
"Connector request failed with status code {}.", response.status_code | ||
) | ||
raise RequestFailureResponseException(response=response) | ||
|
||
|
||
class RequestFailureResponseException(FidesopsException): | ||
|
@@ -242,25 +255,6 @@ def __init__(self, response: Response): | |
self.response = response | ||
|
||
|
||
def log_request_and_response_for_debugging( | ||
prepared_request: PreparedRequest, response: Response | ||
) -> None: | ||
"""Log SaaS request and response in dev mode only""" | ||
if CONFIG.dev_mode: | ||
logger.info( | ||
"\n\n-----------SAAS REQUEST-----------" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol, this is nice to get rid of :) |
||
"\n{} {}" | ||
"\nheaders: {}" | ||
"\nbody: {}" | ||
"\nresponse: {}", | ||
prepared_request.method, | ||
prepared_request.url, | ||
prepared_request.headers, | ||
prepared_request.body, | ||
response._content, # pylint: disable=W0212 | ||
) | ||
|
||
|
||
def get_retry_after(response: Response, max_retry_after: int = 300) -> Optional[float]: | ||
"""Given a Response object, parses Retry-After header and calculates how long we should sleep for""" | ||
retry_after = response.headers.get("Retry-After", None) | ||
|
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.
nice, i think this ends up being quite elegant since these additional pieces of context are composed when passed to a function that's decorated. i really like it! 👏