Allow push_scope to act as a decorator #523

a-d-collins · 2019-10-02T19:18:31Z

Currently the only way to temporarily modify sentry scope attributes (user, level, tag, extra) for a function's scope is to call the function within a use of the push_scope context manager. To streamline this operation, this pull request modifies the ScopeManager class to act as both a context manager and a decorator. In order to make scope attribute modifications possible, sentry_sdk/api.py now has the scope attribute setters: set_tag, set_extra, set_user, and set_level. These setters will apply scope attribute changes to the current scope/scope layer.

a-d-collins · 2019-10-02T19:21:35Z

NOTE: I can't figure out how to add tests for this functionality since there exist limited tests for decorators in the sentry codebase (thus far).

a-d-collins · 2019-10-02T19:23:46Z

Also, since there are not too many custom decorators in the codebase, I wasn't sure what naming convention to follow...

…linter specs

untitaker · 2019-10-03T07:37:01Z

Random thoughts:

I would like to overload push_scope() itself with that purpose. The issue is that push_scope(fn) is already valid and does something else, so we cannot have a decorator @push_scope (only @push_scope()). Let's deprecate this and do this:
```
@push_scope()
def foo():
    pass
```
I do not want to create a purpose-specific API for setting scope data. The JS SDK has global functions for each scope method. I would like to do the same:
```
with push_scope():
    set_extra("foo", "bar")
```

Together they can be composed like this:

@sentry_sdk.push_scope()
def foo():
    sentry_sdk.set_extra("foo", "bar")

a-d-collins · 2019-10-03T14:16:45Z

I like that. So in api.py, along with a new overload of push_scope(), there would be set_tag() and set_extra() which would both utilize Hub.current from api.py's scope.

What about user and level setters? The JS SDK has setUser and setLevel, so I imagine set_user() and set_level() methods would make the most sense?

…) update Hub.push_scope() to allow it to act as both a contentmanager and a decorator, (api.py, utils.py) add new scopemethod api methods {set_tag, set_extra, set_user, set_level} that allow for updating of the current scope

…ator inherit from contextlib.ContextDecorator

…corator with _ScopeManager

…tDecorator compat issue, (sentry_sdk/api.py) update typing to match that of Hub.push_scope()

a-d-collins · 2019-10-05T17:30:57Z

@untitaker, the current code fails on the tests.test_basics.test_scope_leaks_cleaned_up(), however I believe that that test is no longer valid. Now that the scope layering is occurring within ScopeManager.enter(), the following code sample will no longer produce a scope leak:

with push_scope():
    push_scope()

Due to this, I am going to remove that test.

…(tests/test_basics.py) remove unneeded test_scope_leaks_cleaned_up()

untitaker

I'm sorry for not thinking upfront about this, it's tricky to overload push_scope without breaking some API.

I believe we could get the global methods done for now and defer the decision on whether to provide a decorator.

untitaker · 2019-10-06T11:45:09Z

sentry_sdk/scope.py

@@ -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."""


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 set_level"

I can change the docstring and docs.

untitaker · 2019-10-06T11:54:38Z

tests/test_basics.py

-    old_stack = list(Hub.current._stack)
-
-    with push_scope():
-        push_scope()


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. open() also does its work "eagerly".

For context, this is used in third-party integrations in situations where a use of context manager is not possible (search on github for pop_scope_unsafe). They could still manually call __enter__ and ``exit`, but it's a breaking API change in any case.

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.

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.

untitaker · 2019-10-06T11:59:14Z

sentry_sdk/hub.py

@@ -263,6 +268,12 @@ def client(self):
        """Returns the current client on the hub."""
        return self._stack[-1][0]

+    @property
+    def current_scope(self):


If you want this in I would like to call it scope to be consistent with JS

Makes sense.

Actually, I don't think this works because of a line below.

…(sentry_sdk/_compat.py) satisfy linters

untitaker · 2019-10-07T14:26:17Z

@a-d-collins I have thought about this some more and I believe that we need to discuss the point of pushing a scope separately from the global setter functions. Could you split this PR up in two parts:

PR to create global setter functions (and adjust scope API to deprecate properties in favor of setters)
Issue to discuss decorator for pushing a scope. I think we need to get back to the drawing board on this one, particularly in order to understand the usecases better. I believe we might not be on the same page here for when push_scope should be use

a-d-collins · 2019-10-23T02:08:32Z

sentry_sdk/hub.py

-        self._stack.append(new_layer)
-
-        return _ScopeManager(self)
+        return ScopeManager(self)

    scope = push_scope


@untitaker This property here seems to take the namespace of Hub.scope?

master: (sentry_sdk/utils.py) add push_scope_decorator

fbb40b0

a-d-collins closed this Oct 2, 2019

a-d-collins reopened this Oct 2, 2019

a-d-collins added 2 commits October 2, 2019 19:21

master: (sentry_sdk/utils.py) fix iteritems incompatibility

3592c0f

master: (sentry_sdk/utils.py) reformat push_scope_decorator() to fit …

3406239

…linter specs

a-d-collins and others added 5 commits October 4, 2019 16:28

push-scope-decorator: (sentry_sdk/hub.py) have _PushScopeContextDecor…

aaa1c0b

…ator inherit from contextlib.ContextDecorator

push-scope-decorator: (sentry_sdk/hub.py) combine _PushScopeContextDe…

9e0d94b

…corator with _ScopeManager

push-scope-decorator: (hub.py) fix push_scope() typing

27c82c8

master: (sentry_sdk/_compat.py, sentry_sdk/hub.py) resolve py2 Contex…

31d8dfb

…tDecorator compat issue, (sentry_sdk/api.py) update typing to match that of Hub.push_scope()

a-d-collins changed the title ~~Add push_scope_decorator~~ Allow push_scope to act as a decorator Oct 5, 2019

master: (sentry_sdk/scope.py) remove improper uses of @_attr_setter, …

1e52628

…(tests/test_basics.py) remove unneeded test_scope_leaks_cleaned_up()

untitaker requested changes Oct 6, 2019

View reviewed changes

master: (tests/test_basics.py) add test for push_scope as decorator, …

74dedb0

…(sentry_sdk/_compat.py) satisfy linters

a-d-collins commented Oct 23, 2019

View reviewed changes

a-d-collins closed this Oct 23, 2019

This was referenced Oct 23, 2019

Global scope setters #540

Merged

Discussion: Allow push_scope to act as a decorator? #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow push_scope to act as a decorator #523

Allow push_scope to act as a decorator #523

Uh oh!

a-d-collins commented Oct 2, 2019 •

edited

Loading

Uh oh!

a-d-collins commented Oct 2, 2019

Uh oh!

a-d-collins commented Oct 2, 2019

Uh oh!

untitaker commented Oct 3, 2019 •

edited

Loading

Uh oh!

a-d-collins commented Oct 3, 2019

Uh oh!

a-d-collins commented Oct 5, 2019

Uh oh!

untitaker left a comment

Uh oh!

untitaker Oct 6, 2019

Uh oh!

a-d-collins Oct 6, 2019

Uh oh!

untitaker Oct 6, 2019 •

edited

Loading

Uh oh!

a-d-collins Oct 6, 2019

Uh oh!

untitaker Oct 6, 2019

Uh oh!

a-d-collins Oct 6, 2019

Uh oh!

a-d-collins Oct 23, 2019

Uh oh!

untitaker commented Oct 7, 2019

Uh oh!

a-d-collins Oct 23, 2019 •

edited

Loading

Uh oh!

Uh oh!

Allow push_scope to act as a decorator #523

Allow push_scope to act as a decorator #523

Uh oh!

Conversation

a-d-collins commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-d-collins commented Oct 2, 2019

Uh oh!

a-d-collins commented Oct 2, 2019

Uh oh!

untitaker commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-d-collins commented Oct 3, 2019

Uh oh!

a-d-collins commented Oct 5, 2019

Uh oh!

untitaker left a comment

Choose a reason for hiding this comment

Uh oh!

untitaker Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

a-d-collins Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

untitaker Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-d-collins Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

untitaker Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

a-d-collins Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

a-d-collins Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

untitaker commented Oct 7, 2019

Uh oh!

a-d-collins Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a-d-collins commented Oct 2, 2019 •

edited

Loading

untitaker commented Oct 3, 2019 •

edited

Loading

untitaker Oct 6, 2019 •

edited

Loading

a-d-collins Oct 23, 2019 •

edited

Loading