-
Notifications
You must be signed in to change notification settings - Fork 161
expose OpenTracing and API versioning capabilities #16
Changes from 1 commit
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 |
---|---|---|
|
@@ -46,7 +46,7 @@ def test_start_trace(self): | |
with tracer.start_trace(operation_name='Fry', | ||
tags={'birthday': 'August 14 1974'}) as span: | ||
span.log_event('birthplace', | ||
payload={'hospital': 'Brooklyn Pre-Med Hospital', | ||
payload={'hospital': 'Brooklyn Pre-Med Hospital', | ||
'city': 'Old New York'}) | ||
tracer.close() | ||
|
||
|
@@ -146,3 +146,8 @@ def test_binary_propagation(self): | |
trace_attributes=bin_attrs | ||
) as reassembled_span: | ||
reassembled_span.set_trace_attribute('middle-name', 'Rodriguez') | ||
|
||
def test_implementation_id(self): | ||
impl_id = self.tracer().implementation_id() | ||
assert len(impl_id.name) > 0 | ||
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. not sure what else to check here... |
||
assert len(impl_id.version) > 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
|
||
from __future__ import absolute_import | ||
from concurrent.futures import Future | ||
from collections import namedtuple | ||
from .span import Span | ||
from .propagator import SpanPropagator | ||
|
||
|
@@ -82,3 +83,24 @@ def close(self): | |
fut = Future() | ||
fut.set_result(True) | ||
return fut | ||
|
||
def implementation_id(self): | ||
"""Returns the ImplementationID for this OpenTracing implementation. | ||
|
||
:return: An ImplementationID instance that describes this OpenTracing | ||
implementation. | ||
""" | ||
return ImplementationID('noop', '1.0.0') | ||
|
||
|
||
# Per collections.namedtuple, the below defines a class called ImplementationID | ||
# which can be initialized via positional parameters or a kwargs-style | ||
# initializer. | ||
# | ||
# dapper_impl = ImplementationID('dapper', '0.1.2') | ||
# zipkin_impl = ImplementationID(name='zipkin', version='0.3.4') | ||
# | ||
ImplementationID = namedtuple('ImplementationID', 'name version') | ||
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 like 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. sgtm - it's also immutable. Not sure we need the comment in L96-98. |
||
|
||
# The SemVer (http://semver.org/) of the OpenTracing Python API. | ||
OPENTRACING_PYTHON_SEMVER = '0.9.0' | ||
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. @bcronin do we need anything fancier than this? I think the biggest challenge will be remembering to update it :-/ Of course we can automate that process to a certain extent... 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 would put it at the top of the file.
:-) I remember seeing somewhere a lib was taking a hash of its public API and having a unit test comparing that hash with a constant, and a comment in upper case "if this hash changed the version needs to change". If I have any grey hair, that's when it started. 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. (Preface: I do not know python very well, so this might be just a language misunderstanding on my part...) What I was imagining was given a At a high-level the problem is the case (impossible in some setups, but not all) were a single logical application has been bound, for one reason or another, to multiple versions of OpenTracing (via use of third-party libraries, nested dependencies, dynamically loaded code, etc.). This case is important because removing the multiple versions is not always something within an individual developer's control (e.g. nested dependencies and third-party code). In Node.js for example, as far as I know it is fully possible to have two references to two different versions of the same package within a single logical application or service (there will be two distinct copies of the package within the loaded code; to help imagine the real-world scenario think of an application that relies on a framework that relies on third-party extensions that rely on third-party libraries -- and in that complex dependency tree someone has not updated their code to the latest version of OpenTracing but others have). The version check needs to be done on the actual To clarify this is not Node.js specific, consider a C++ application that dynamically loads a library (i.e. a DLL/.so plugin of some sort) that, as part of the plugin interface, takes a handle to the active Tracer. It may be interested in knowing the version of that runtime Tracer object it has been handed, not the version of OpenTracing the DLL itself was compiled against, to be sure the host application is compatible. A global check would only check what it had been compiled against. I certainly wouldn't argue these are going to be a common cases but to achieve the intent, I believe the version info has to be a property of the Tracer object, not a package/module/library-level property. 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. @bcronin I see what you're saying. If we're going through the trouble of including this API in the first place, we may as well do it right. I'm not at my laptop right now, but I will think about it... It's annoying that Tracer impls need not actually inherit from Tracer (due to the usual dynamic typing situation)... If that weren't the case, this would be straightforward! Maybe we should just require it in a clearly worded comment or something... |
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 delinted and found some spaces/tabs things... probably my copy of vim)