Skip to content

Fix last_event_id() in nested scopes #3114

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix last_event_id() in nested scopes #3114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions sentry_sdk/scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,6 @@ def __init__(self, ty=None, client=None):
incoming_trace_information = self._load_trace_data_from_env()
self.generate_propagation_context(incoming_data=incoming_trace_information)

# self._last_event_id is only applicable to isolation scopes
self._last_event_id = None # type: Optional[str]

Comment on lines -211 to -213
Copy link
Member

Choose a reason for hiding this comment

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

While I agree that it likely makes sense to move these lines to the clear function, I believe this change should be considered in a separate PR, since it does not appear to be needed to fix #3113; the tests you added pass even if I undo this change.

def __copy__(self):
# type: () -> Scope
"""
Expand Down Expand Up @@ -244,6 +241,8 @@ def __copy__(self):

rv._profile = self._profile

rv._last_event_id = self._last_event_id

return rv

@classmethod
Expand Down Expand Up @@ -677,6 +676,9 @@ def clear(self):

self._propagation_context = None

# self._last_event_id is only applicable to isolation scopes
self._last_event_id = None # type: Optional[str]

Comment on lines +679 to +681
Copy link
Member

Choose a reason for hiding this comment

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

Likely belongs in separate PR, see previous comment.

@_attr_setter
def level(self, value):
# type: (LogLevelStr) -> None
Expand Down
8 changes: 8 additions & 0 deletions tests/test_basics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -800,3 +800,11 @@ def test_last_event_id_transaction(sentry_init):
pass

assert last_event_id() is None, "Transaction should not set last_event_id"


def test_last_event_id_scope(sentry_init):
sentry_init(enable_tracing=True)

# Should not crash
with push_scope() as scope:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with push_scope() as scope:
with isolation_scope() as scope:

push_scope is deprecated since version 2.0.0, so let's use the new isolation_scope function here, instead. Using isolation_scope also fails on master but passes on this branch.

assert scope.last_event_id() is None
4 changes: 4 additions & 0 deletions tests/test_scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def test_copying():
s1.set_tag("foo", "bar")

s2 = copy.copy(s1)
# Check all attributes are copied
for name in set(Scope.__slots__) - {"client"}:
getattr(s2, name)

Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to add this check to a separate unit test, since it is somewhat unrelated to the tag copying checks that this existing test is checking. The comment should also be updated to something like "Check that all attributes, except client, are present on the new scope", as the check only verifies the attributes' presence, and does not check whether the values were actually copied.

assert "foo" in s2._tags

s1.set_tag("bam", "baz")
Expand Down
Loading