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

Implement PEP3134 to discover underlying problems with python3 #355

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Implement PEP3134 to discover underlying problems with python3 #355

merged 1 commit into from
Sep 26, 2022

Conversation

lehmat
Copy link
Contributor

@lehmat lehmat commented Sep 19, 2022

Issue #, if available:
#354

Description of changes:
Implement PEP3134 through six.raise_from to support python2 and python3.

Also move one import to top of the module to better comply with pylint (C0413:wrong-import-position)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lehmat lehmat requested a review from a team as a code owner September 19, 2022 07:53

if sys.version_info >= (3, 0, 0):
from urllib.parse import urlparse, uses_netloc
else:
from urlparse import urlparse, uses_netloc

import wrapt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved upwards to remove pylint:C0413

@lehmat
Copy link
Contributor Author

lehmat commented Sep 19, 2022

This ticket is tied to the following troubleshooting and discovering this issue: Pynamodb 1073

Copy link
Contributor

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

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

LGTM!

@carolabadeer carolabadeer merged commit 4660169 into aws:master Sep 26, 2022
@lehmat lehmat deleted the feature/354 branch September 27, 2022 05:09
@michael-k
Copy link
Contributor

michael-k commented Nov 18, 2022

I've never seen raise e from e where both the exception and the cause are the same. When playing with that in IPython, I don't see any benefit either. Instead it adds a lot of noise:

Python 3.11.0 (main, Nov 12 2022, 07:10:51) [GCC 11.2.1 20220219]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.6.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: try:
   ...:     raise ValueError("foo")
   ...: except ValueError as exc:
   ...:     print("bar")
   ...:     raise exc from exc
   ...:
bar
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
    [... skipping hidden 1 frame]

Cell In [1], line 5
      4 print("bar")
----> 5 raise exc from exc

Cell In [1], line 2
      1 try:
----> 2     raise ValueError("foo")
      3 except ValueError as exc:

ValueError: foo

The above exception was the direct cause of the following exception:

ValueError                                Traceback (most recent call last)
    [... skipping hidden 1 frame]

Cell In [1], line 5
      4 print("bar")
----> 5 raise exc from exc

Cell In [1], line 2
      1 try:
----> 2     raise ValueError("foo")
      3 except ValueError as exc:

ValueError: foo

The above exception was the direct cause of the following exception:

ValueError                                Traceback (most recent call last)
Cell In [1], line 5
      3 except ValueError as exc:
      4     print("bar")
----> 5     raise exc from exc

Cell In [1], line 2
      1 try:
----> 2     raise ValueError("foo")
      3 except ValueError as exc:
      4     print("bar")

ValueError: foo

PEP 3134 doesn't mention such a use case either.
@lehmat Can you elaborate?

raise
In [2]: raise ValueError("foo")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In [2], line 1
----> 1 raise ValueError("foo")

ValueError: foo
try-catch-raise without from
In [3]: try:
   ...:     raise ValueError("foo")
   ...: except ValueError as exc:
   ...:     print("bar")
   ...:     raise exc
   ...:
bar
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In [3], line 5
      3 except ValueError as exc:
      4     print("bar")
----> 5     raise exc

Cell In [3], line 2
      1 try:
----> 2     raise ValueError("foo")
      3 except ValueError as exc:
      4     print("bar")

ValueError: foo

@lehmat
Copy link
Contributor Author

lehmat commented Nov 18, 2022

This PR was made because there was underlying issue hidden by the missing raise from syntax in aws-xray-sdk core. If this would have been implemented previously, the underlying issue would have been seen without disabling xray in the application code.

What was seen in logs prior to this PR

What this PR improves

  • Before: The root cause is found at File "aws_xray_sdk/core/recorder.py", line 436, in record_subsegment, with invalid quote_from_bytes arguments
  • After: The root cause if found at File sentry_sdk/tracing_utils.py", line 541 with invalid quote_from_bytes argument

Essentially it will give that The above exception was the direct cause of the following exception: to be pointing at underlying issue at sentry-sdk instead of stopping the traceback at aws_xray_sdk

Reference of the underlying issue that was actually the issue:

@michael-k
Copy link
Contributor

I saw both issues and read through them, but that doesn't explain why this is an issue in the first place. A simple re-raise should not omit parts of the stacktrace. In pynamodb/PynamoDB#1073 you said that you “disable[d] AWS X-ray to get the proper root cause to popup.” Did you also try it with this patch and X-Ray enabled? I'm not convinced that this actually solves your problem.

Maybe X-Ray accessing the stacktrace has something to do with it? I played around with that, but found no hint that would support this theory. But my toy stacktraces were small.

From raise's documentation:

Changed in version 3.11: If the traceback of the active exception is modified in an except clause, a subsequent raise statement re-raises the exception with the modified traceback. Previously, the exception was re-raised with the traceback it had when it was caught.

getsentry/sentry-python#1626 mentions Python 3.9, therefore the re-raise shouldn't have changed the traceback.

@lehmat
Copy link
Contributor Author

lehmat commented Nov 18, 2022

After reviewing your comments I agree with you that this does not solve the problem.

Did you also try it with this patch and X-Ray enabled?

No, manipulating dependencies would be troublesome with our way of deploying lambda

Essentially this would behave as it did (with sentry-sdk[flask]==1.9.8)

import os
import serverless_wsgi

import sentry_sdk
from flask import Flask
import boto3
from aws_xray_sdk.core import xray_recorder
from aws_xray_sdk.core import patch_all

xray_recorder.configure(service='Test')
patch_all()


app = Flask(__name__)


sentry_sdk.init(
    dsn=os.getenv("SENTRY_DSN"),  # need to have some sane here to actually make sentry work
    release=1,  # This was the breaking on sentry-sdk, integer release causing it
    traces_sample_rate=1,
)

@app.route("/hello")
def hello_world():
    segment = xray_recorder.begin_segment('segment_name')
    boto3.client('sts').get_caller_identity().get('Account')
    xray_recorder.end_segment()
    return "<p>Hello, World!</p>"

def handler(event, context):
    return serverless_wsgi.handle_request(app, event, context)

michael-k added a commit to michael-k/aws-xray-sdk-python that referenced this pull request Nov 29, 2022
carolabadeer added a commit that referenced this pull request Dec 27, 2022
#355)" (#371)

This reverts commit 4660169.

Closes #370

Co-authored-by: Carol Abadeer <[email protected]>
Co-authored-by: Prashant Srivastava <[email protected]>
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