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

Insitu query logging & request timeouts #207

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

RKuttruff
Copy link
Contributor

Improved the logging for insitu queries to show when the query starts + its URL and then when the query ends, its URL (for correlation), status code, and elapsed time.

@skorper skorper self-requested a review October 24, 2022 22:23

try:
if session is not None:
response = session.get(next_page_url, params=params, timeout=(15.05, 331))
Copy link
Member

Choose a reason for hiding this comment

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

What's the justification of setting these values separately? Also, why 331 and not 300 (the gateway timeout we have configured)? Also curious why you chose 15.05

Also, we should probably avoid magic numbers and assign these values to variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by the first question.

I chose the numbers (primarily 15.05) since the documentation for setting timeouts recommends setting it to a little bit higher than a multiple of 3, so I picked one admittedly arbitrarily.

Copy link
Member

Choose a reason for hiding this comment

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

I meant why set timeout=(15.05, 331) rather than timeout=331?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First one is connect timeout, second is read timeout.
I suppose it would be beneficial to fail quickly if we can't even connect rather than wait a whole 5 minutes

@RKuttruff RKuttruff changed the title Insitu query logging Insitu query logging & request timeouts Nov 30, 2022
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.

2 participants