-
Notifications
You must be signed in to change notification settings - Fork 395
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
[requests] add unpatch and double-patch protection #404
Conversation
_u(requests.Session, 'request') | ||
|
||
|
||
def _session_initializer(func, instance, args, kwargs): |
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.
Not a big deal now, but it's required later
405f3f6
to
9c78018
Compare
eq_(len(spans), 1) | ||
s = spans[0] | ||
eq_(s.get_tag(http.METHOD), 'GET') | ||
eq_(s.get_tag(http.STATUS_CODE), '500') | ||
eq_(s.error, 1) | ||
|
||
|
||
def get_traced_session(): |
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.
Removed since a not monkey-patched class was used. With this new test suite, we ensure that patch()
really updates the Session
class.
from .patch import TracedSession, patch | ||
__all__ = ['TracedSession', 'patch'] | ||
from .patch import TracedSession, patch, unpatch | ||
__all__ = ['TracedSession', 'patch', 'unpatch'] |
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.
Is the deal to expose and document the unpatch
method for this integration?
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.
No actually no, it has been exposed only for testing purposes. We probably don't want to suggest patching and unpatching at runtime. Anyway there is a plan to refactor the patching system, since we should start to introduce settings for each integration.
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.
Clean, gtm.
Overview
patch()
unpatch()
to remove tracing__init__
so that we can set later (another PR) settings from env vars