-
Notifications
You must be signed in to change notification settings - Fork 554
Allow push_scope to act as a decorator #523
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
Changes from all commits
fbb40b0
3592c0f
3406239
4b6d510
aaa1c0b
9e0d94b
27c82c8
31d8dfb
1e52628
74dedb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
from datetime import datetime | ||
from contextlib import contextmanager | ||
|
||
from sentry_sdk._compat import with_metaclass | ||
from sentry_sdk._compat import with_metaclass, ContextDecorator | ||
from sentry_sdk.scope import Scope | ||
from sentry_sdk.client import Client | ||
from sentry_sdk.tracing import Span | ||
|
@@ -123,15 +123,20 @@ def main(cls): | |
return GLOBAL_HUB | ||
|
||
|
||
class _ScopeManager(object): | ||
class ScopeManager(ContextDecorator): | ||
def __init__(self, hub): | ||
# type: (Hub) -> None | ||
self._hub = hub | ||
self._original_len = len(hub._stack) | ||
self._layer = hub._stack[-1] | ||
|
||
def __enter__(self): | ||
# type: () -> Scope | ||
client, scope = self._hub._stack[-1] | ||
new_layer = (client, copy.copy(scope)) | ||
self._hub._stack.append(new_layer) | ||
|
||
self._original_len = len(self._hub._stack) | ||
self._layer = self._hub._stack[-1] | ||
|
||
scope = self._layer[1] | ||
assert scope is not None | ||
return scope | ||
|
@@ -263,6 +268,12 @@ def client(self): | |
"""Returns the current client on the hub.""" | ||
return self._stack[-1][0] | ||
|
||
@property | ||
def current_scope(self): | ||
# type: () -> Scope | ||
"""Returns the current scope on the hub.""" | ||
return self._stack[-1][1] | ||
|
||
def last_event_id(self): | ||
# type: () -> Optional[str] | ||
"""Returns the last event ID.""" | ||
|
@@ -448,7 +459,7 @@ def start_span( | |
def push_scope( | ||
self, callback=None # type: Optional[None] | ||
): | ||
# type: (...) -> ContextManager[Scope] | ||
# type: (...) -> ScopeManager | ||
pass | ||
|
||
@overload # noqa | ||
|
@@ -461,14 +472,14 @@ def push_scope( | |
def push_scope( # noqa | ||
self, callback=None # type: Optional[Callable[[Scope], None]] | ||
): | ||
# type: (...) -> Optional[ContextManager[Scope]] | ||
# type: (...) -> Optional[ScopeManager] | ||
""" | ||
Pushes a new layer on the scope stack. | ||
|
||
:param callback: If provided, this method pushes a scope, calls | ||
`callback`, and pops the scope again. | ||
|
||
:returns: If no `callback` is provided, a context manager that should | ||
:returns: If no `callback` is provided, a ContextDecorator that should | ||
be used to pop the scope again. | ||
""" | ||
|
||
|
@@ -477,11 +488,7 @@ def push_scope( # noqa | |
callback(scope) | ||
return None | ||
|
||
client, scope = self._stack[-1] | ||
new_layer = (client, copy.copy(scope)) | ||
self._stack.append(new_layer) | ||
|
||
return _ScopeManager(self) | ||
return ScopeManager(self) | ||
|
||
scope = push_scope | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @untitaker This property here seems to take the namespace of Hub.scope? |
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,11 @@ def level(self, value): | |
"""When set this overrides the level.""" | ||
self._level = value | ||
|
||
def set_level(self, value): | ||
# type: (Optional[str]) -> None | ||
"""When set this overrides the level.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is no longer a property this docstring should be rephrased. Also please change the level property docs to say "deprecated in favor of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change the docstring and docs. |
||
self._level = value | ||
|
||
@_attr_setter | ||
def fingerprint(self, value): | ||
# type: (Optional[List[str]]) -> None | ||
|
@@ -144,6 +149,11 @@ def user(self, value): | |
"""When set a specific user is bound to the scope.""" | ||
self._user = value | ||
|
||
def set_user(self, value): | ||
# type: (Dict[str, Any]) -> None | ||
"""When set a specific user is bound to the scope.""" | ||
self._user = value | ||
|
||
@property | ||
def span(self): | ||
# type: () -> Optional[Span] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
add_breadcrumb, | ||
last_event_id, | ||
Hub, | ||
set_level, | ||
) | ||
from sentry_sdk.integrations.logging import LoggingIntegration | ||
|
||
|
@@ -174,6 +175,30 @@ def _(scope): | |
assert Hub.current._stack[-1][1] is outer_scope | ||
|
||
|
||
def test_push_scope_as_decorator(sentry_init, capture_events): | ||
sentry_init() | ||
events = capture_events() | ||
|
||
Hub.current.bind_client(None) | ||
|
||
outer_scope = Hub.current._stack[-1][1] | ||
|
||
@push_scope() | ||
def foo(): | ||
set_level("warning") | ||
try: | ||
1 / 0 | ||
except Exception as e: | ||
capture_exception(e) | ||
|
||
foo() | ||
|
||
event, = events | ||
assert outer_scope._level != "warning" | ||
assert event["level"] == "warning" | ||
assert "exception" in event | ||
|
||
|
||
def test_breadcrumbs(sentry_init, capture_events): | ||
sentry_init(max_breadcrumbs=10) | ||
events = capture_events() | ||
|
@@ -240,23 +265,6 @@ def test_client_initialized_within_scope(sentry_init, caplog): | |
assert record.msg.startswith("init() called inside of pushed scope.") | ||
|
||
|
||
def test_scope_leaks_cleaned_up(sentry_init, caplog): | ||
caplog.set_level(logging.WARNING) | ||
|
||
sentry_init(debug=True) | ||
|
||
old_stack = list(Hub.current._stack) | ||
|
||
with push_scope(): | ||
push_scope() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn, I didn't consider that this usecase no longer works. I'll have to think about this more. Right now I believe this is the less expected behavior, after all e.g. For context, this is used in third-party integrations in situations where a use of context manager is not possible (search on github for The scope cleanup described here has to continue working in any case because of concurrency issues in async envs that we need to mitigate against. For example, some async environment may share the hub across threads, which incurs data races left and right but push_scope and pop_scope_unsafe should crash as little as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I definitely understand that scope cleanup needs to happen in async envs. However, if it is as critical as you say that push_scope() continues working the "eager" way it does now AND have it work as a context manager AND like this @sentry_sdk.push_scope()
def foo():
sentry_sdk.set_tag("key", "value") Then I haven't thought of how to make it work, it's not possible, or something else may have to give. From my outsider's perspective the easiest solution is to deprecate push_scope(callback) functionality + alter the Hub and ScopeManager classes so that the following examples would all work: @sentry_sdk.push_scope
def foo():
sentry_sdk.set_tag("key", "value")
with sentry_sdk.push_scope():
print("pushed scope!")
scope = sentry_sdk.push_scope() but... that feels like a very major change to the api. |
||
|
||
assert Hub.current._stack == old_stack | ||
|
||
record, = (x for x in caplog.records if x.levelname == "WARNING") | ||
|
||
assert record.message.startswith("Leaked 1 scopes:") | ||
|
||
|
||
def test_scope_popped_too_soon(sentry_init, caplog): | ||
caplog.set_level(logging.ERROR) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want this in I would like to call it
scope
to be consistent with JSThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think this works because of a line below.