Skip to content
This repository was archived by the owner on Jun 13, 2023. It is now read-only.

fix(tornado.py): fix mishandling of 404s#317

Merged
ranrib merged 12 commits intomasterfrom
tornado-updates
Jan 12, 2021
Merged

fix(tornado.py): fix mishandling of 404s#317
ranrib merged 12 commits intomasterfrom
tornado-updates

Conversation

@ranrib
Copy link
Copy Markdown
Contributor

@ranrib ranrib commented Jan 12, 2021

In addition:

  1. capturing redis response
  2. support older redis versions
  3. Fixing mishandling of 404 in aiohttp
  4. fixing log-correlation that was always handled as False (since trace_factory was not initialized)
  5. adding some debug prints to Tornado and aiohttp

@ranrib ranrib requested a review from a team as a code owner January 12, 2021 11:28
@ranrib ranrib requested a review from maorlx January 12, 2021 11:28
Comment thread epsagon/utils.py
Comment on lines +105 to +108
if os.getenv('EPSAGON_LOGGING_TRACING_ENABLED'):
return os.getenv('EPSAGON_LOGGING_TRACING_ENABLED').upper() == 'TRUE'

return True
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if os.getenv('EPSAGON_LOGGING_TRACING_ENABLED'):
return os.getenv('EPSAGON_LOGGING_TRACING_ENABLED').upper() == 'TRUE'
return True
return os.getenv('EPSAGON_LOGGING_TRACING_ENABLED', 'TRUE').upper() == 'TRUE'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

<3

Comment thread epsagon/wrappers/aiohttp.py Outdated
print_debug('[aiohttp] got response')
except Exception as exception: # pylint: disable=W0703
# Ignoring 404s
if type(exception).__name__ == 'HTTPNotFound':
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we import the specific exception class and compare the types objects?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

<3

Comment thread epsagon/modules/redis.py Outdated
_single_wrapper
)
# Version < 3.0
wrapt.wrap_function_wrapper(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to make sure, in newer versions this call won't explode - right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's right, this code is failsafe


# Ignoring 404s
if getattr(instance, '_status_code', None) == 404:
print_debug('Ignoring 404 Tornado request')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should pop the trace from trace_factory (and prevent memory leaks for those 404 traces)

Comment thread epsagon/runners/tornado.py Outdated
body
)
except Exception as exception: # pylint: disable=broad-except
print_debug('Could not extract body: {}'.format(exception))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
print_debug('Could not extract body: {}'.format(exception))
print_debug('Could not extract request body: {}'.format(exception))

if response_body:
body = response_body
if isinstance(body, list):
body = body[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in which cases is the body list? (wondering whether we want to collect all the list items)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems to be always a list from what I've seen, and always a single element

@ranrib ranrib requested a review from maorlx January 12, 2021 14:28
maorlx
maorlx previously approved these changes Jan 12, 2021
@ranrib ranrib merged commit b1ca110 into master Jan 12, 2021
@ranrib ranrib deleted the tornado-updates branch January 12, 2021 15:04
@enoodle
Copy link
Copy Markdown
Contributor

enoodle commented Jan 12, 2021

🎉 This PR is included in version 1.61.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants