-
Notifications
You must be signed in to change notification settings - Fork 505
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
Expose the scope getters to top level API and use them everywhere #3357
Conversation
4b39048
to
c9598b1
Compare
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 14758 tests with View the full list of failed testspy3.6-boto3-v1.12
py3.6-common
py3.6-gevent
py3.7-aiohttp-v3.4
py3.7-boto3-v1.23
py3.7-common
py3.7-httpx-v0.23
py3.8-aiohttp-latest
py3.8-common
py3.8-gevent
py3.9-aiohttp-v3.8
py3.9-common
py3.9-httpx-latest
py3.9-httpx-v0.27
|
c9598b1
to
1fb1170
Compare
88eaf8a
to
e67291c
Compare
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.
LGTM, see small comments
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.
Looks good. Just a few nitpicks
@@ -512,15 +512,15 @@ def get_traceparent(self, *args, **kwargs): | |||
return traceparent | |||
|
|||
# Fall back to isolation scope's traceparent. It always has one | |||
return Scope.get_isolation_scope().get_traceparent() | |||
return self.get_isolation_scope().get_traceparent() |
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.
In scope.py
here we use self.get_x()
because in the future we will have a check in api.get_x()
for otel or sentry and we do not want to call the check all the time?
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.
yes, there will be a PotelScope
class that will inherit from Scope
but the internal getters/setters will be different
2a48e76
to
c577b31
Compare
* Going forward, we might have 2 different scope implementations so we can't have the `Scope` class being called everywhere directly since this will be abstracted away.
Co-authored-by: Ivana Kellyer <[email protected]>
c577b31
to
1d6420c
Compare
…tsentry#3357) * Expose the scope getters to top level API and use them everywhere * Going forward, we might have 2 different scope implementations so we can't have the `Scope` class being called everywhere directly since this will be abstracted away. * Update CHANGELOG.md Co-authored-by: Ivana Kellyer <[email protected]> * remove Scope._capture_internal_exception * review fixes * remove staticmethod * Fix sphinx circular import bs --------- Co-authored-by: Ivana Kellyer <[email protected]>
We removed this line in getsentry#3354 since it is no longer needed, but it was apparently accidentally added back in getsentry#3357.
Going forward, we might have 2 different scope implementations for otel so we
can't have the
Scope
class being called everywhere directly since thiswill be abstracted away.
The refactor had 2 main goals
import Scope
outside of the main init/api filesScope.*
calls outside the actual currentScope
impl and replace them with top level api everywhere__init__
andapi
files since some methods should've been inapi