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

fix(api): Fix tracing TypeError for static and class methods #2559

Merged
merged 18 commits into from Jan 5, 2024

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Dec 1, 2023

Fixes TypeError that occurred when static or class methods, which were passed in the functions_to_trace argument when initializing the SDK, were called on an instance.

We also need to update docs to specify order when using sentry_sdk.trace decorator (as in the workaround I suggested on the issue)

Fixes GH-2525

Comment on lines 123 to 150
def test_staticmethod_patching(sentry_init):
test_staticmethod_name = "test_decorator_py3.TestClass.static"
assert (
".".join([TestClass.static.__module__, TestClass.static.__qualname__])
== test_staticmethod_name
), "The test static method was moved or renamed. Please update the name accordingly"

sentry_init(functions_to_trace=[{"qualified_name": test_staticmethod_name}])

for instance_or_class in (TestClass, TestClass()):
with patch_start_child() as fake_start_child:
assert instance_or_class.static(1) == 1
fake_start_child.assert_called_once()


def test_classmethod_patching(sentry_init):
test_classmethod_name = "test_decorator_py3.TestClass.class_"
assert (
".".join([TestClass.class_.__module__, TestClass.class_.__qualname__])
== test_classmethod_name
), "The test class method was moved or renamed. Please update the name accordingly"

sentry_init(functions_to_trace=[{"qualified_name": test_classmethod_name}])

for instance_or_class in (TestClass, TestClass()):
with patch_start_child() as fake_start_child:
assert instance_or_class.class_(1) == (TestClass, 1)
fake_start_child.assert_called_once()
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably move these to test_basics since they are not specific to the test decorator and should also run on Python 2 (put them here originally when I thought I would fix the bug differently)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me either way

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

👍🏻

@szokeasaurusrex szokeasaurusrex changed the title Fix tracing TypeError for static and class methods fix(api): Fix tracing TypeError for static and class methods Dec 27, 2023
@@ -0,0 +1,48 @@
from unittest import mock
Copy link
Member Author

Choose a reason for hiding this comment

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

This file moved from tests/tracing/test_decorator_py3.py

@szokeasaurusrex
Copy link
Member Author

I decided to move the tests I created to test_basics.py, and while doing so, I realized I could also refactor the tracing decorator tests to reuse common logic, rather than repeating extremely similar code snippets in multiple locations. I have added these improvements to this PR, and would appreciate a re-review.

@@ -141,7 +141,7 @@ async def sentry_app_handle(self, request, *args, **kwargs):
transaction.set_http_status(response.status)
return response

Application._handle = sentry_app_handle # type: ignore[method-assign]
Copy link
Member Author

Choose a reason for hiding this comment

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

mypy was complaining about the type: ignore comments in this file, so I had to make changes here, even though the changes are unrelated to the rest of the PR

Copy link
Member

@antonpirker antonpirker 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

@szokeasaurusrex szokeasaurusrex merged commit 8bd2f46 into master Jan 5, 2024
123 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/fastapi-typeerror branch January 5, 2024 10:03
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.

Using defined span creation on static class method cause TypeError
3 participants