Skip to content
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

[LOGGING_4864] Consider timeout errors and add logging debug statements #39

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

jsubirat
Copy link
Contributor

Description

A customer notified us that the retry mechanism was not properly considering TimeoutErrors, and even opened a ticket suggesting a solution. After inspecting the issue, we found that what was really performing the timeout was the ClientSession established for the whole lambda execution, which was set to tool low (3 seconds, I wonder if it was meant to be 30 instead). This PR fixes this issue by:

  • Defining a timeout of 3 seconds for individual POST requests
  • Defining a session timeout which is the sum of the expected individual POST requests (assuming the worst case of having a timeout in each of them), the backoffs between them and an additional time buffer of 1 second, to account for the processing time needed to create the payloads/requests.
  • The customer also complained that it was rather difficult to debug what was failing of our lambda, so we added a debug mode that enables log.debug statements when retries are performed, as well as providing more details on the errors taking place.

Related ticket(s)

https://newrelic.atlassian.net/browse/LOGGING-4864
https://newrelic.atlassian.net/browse/LOGGING-4700

Tests

  • The PR includes new unit tests for the added/modified functionalities.
  • Manual tests have been performed in AWS lambda, using a fake API introducing controlled timeouts.

Copy link
Contributor

@MattWhelan MattWhelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. If you could just run black to reformat your changes to get past the lint error, we can merge this.

MattWhelan
MattWhelan previously approved these changes Apr 13, 2021
@MattWhelan
Copy link
Contributor

@jsubirat Please amend your commits, sign them, and force-push.

serverless.yml Outdated Show resolved Hide resolved
src/function.py Outdated Show resolved Hide resolved
src/function.py Show resolved Hide resolved
src/function.py Show resolved Hide resolved
src/function.py Show resolved Hide resolved
src/function.py Show resolved Hide resolved
@jsubirat jsubirat force-pushed the LOGGING-4864_consider_timeouts_to_retry_sending branch 2 times, most recently from 97dd2f3 to 5709790 Compare April 14, 2021 11:18
@jsubirat jsubirat force-pushed the LOGGING-4864_consider_timeouts_to_retry_sending branch from 5709790 to 1ba299a Compare April 14, 2021 11:26
@MattWhelan MattWhelan merged commit f34187c into master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants