-
Notifications
You must be signed in to change notification settings - Fork 161
expose OpenTracing and API versioning capabilities #16
Conversation
@@ -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', |
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)
@bcronin how about this latest version? |
The update addresses the "two copies" scenario I was concerned with, so now I'm descending to the next level of detail at this point (i.e. naming)... I can't help but think it'd be a lot more straightforward to just have: def version():
return '0.9.0'
All in all, I think |
Having slept on this, the only thing I really want to expose at this point is the semver of the per-platform OpenTracing API. I don't want the OT implementor to have to remember to update same, hence the use of the constant rather than a method which should be overridden. Note that the per-platform semvers will not be in lock-step across the various platforms (if we refactor the API in one language, its semver will need to change, but there's no reason that the others must follow suit). Someday it would be nice to semver the platform-indepedent OpenTracing specification, but for now it's not formal enough to make a semver meaningful (in my opinion, anyway). Thoughts? In the meantime, I will trash the impl-id stuff entirely. |
(trashing complete!) |
Ah, ok - the intent is to expose the platform API version which may be independent of the spec version. That still allows for "two copies" case to be handled, so I'm fine with this. |
Github confused me with the ordering of the comments...
|
|
But I have to admit that I haven't had the pleasure of writing such spaghetti code in dynamic langauges and checking versions, so I don't want to |
Thoughts? I am not attached to this PR but wanted to take a stab at opentracing/opentracing.io#33 without adding a ton of complexity. |
|
JS is a free-for-all... @bcronin actually understands it (I do not; or not well). I'll sleep on this whole thing... part of me wants to back it out until we have an actual specific problem to solve in the wild. |
The spirit of #33 was to provide a simple means of accessing the version of the OpenTracing standard being implemented against. If there also needs to be a separate notion of a platform version that can vary independently, frankly I think that adds additional complexity counter to the original goal and I suspect the intended value gets lost. I think there's some (bear with a bit of soapboxing here to make myself feel better) danger in waiting until there's an actual specific problem to solve in the wild as that sort is missing a large part of adding versioning from the start. (Also, for the record most of my forward/backwards compatibility experience here has to do with drivers and plug-ins in C++ SDK architectures, not JavaScript, but the API scope here is much smaller so I'm totally willing to concede that I may be overthinking it.) Anyway, my soapboxing done and stepping back, there are things I'd rather prioritize here and, in practice, even if it's not elegant, people almost always find some way to work around version incompatibilities, so I think it'd be net-best to close #33. |
(withdrawing per the fate of opentracing/opentracing.io#33) |
Per opentracing/opentracing.io#33 and opentracing/opentracing-go#51 (comment)
@bcronin @yurishkuro