-
Notifications
You must be signed in to change notification settings - Fork 653
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
Adding trace.get_tracer #430
Adding trace.get_tracer #430
Conversation
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, I agree this is a better API.
It's fairly common to need to retrieve tracers, and as such adding the additional tracer_source() call to every retrieval of a tracer can add to a lot of extra boilerplate at minimal semantic value. It would be uncommon for one to use multiple trace_source objects, as typically one would want all tracers to be created and behave in a similar way (e.g. passed to the same span processor).
Co-Authored-By: Chris Kleinknecht <libc@google.com>
Co-Authored-By: Chris Kleinknecht <libc@google.com>
d32d7c3
to
b4ec34e
Compare
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
+ Coverage 88.19% 88.27% +0.07%
==========================================
Files 41 41
Lines 2042 2047 +5
Branches 234 234
==========================================
+ Hits 1801 1807 +6
Misses 170 170
+ Partials 71 70 -1
Continue to review full report at Codecov.
|
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.
I agree this simplifies the API.
"""trace.get_tracer should proxy to the global tracer source.""" | ||
from_global_api = trace.get_tracer("foo") | ||
from_tracer_api = trace.tracer_source().get_tracer("foo") | ||
self.assertEqual(from_global_api, from_tracer_api) |
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.
assertIs
?
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.
Thanks for making this change! 👍
@@ -663,6 +679,10 @@ def tracer_source() -> TracerSource: | |||
except TypeError: |
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.
Why do we even catch TypeErrors (and only TypeErrors) here?
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.
To catch exceptions when instantiating an abstract base class
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.
Thoughts on catching any exception here? the reason I added this was because I ran into behavior where I wasn't overriding the default tracer, and found it's because I had a bug in my unit test (caused by importllib.reload recreating all instances).
I feel like I'd want more feedback, but maybe that's already handled by the lack of an exception handler (error will just bubble up).
closes open-telemetry#429 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
As a convenience method to retrieve a tracer, introducing trace.get_tracer.
Overall I think this removes an extra element that is strongly implied.