Skip to content
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

Add API to query OpenTracing version at runtime #33

Closed
bcronin opened this issue Jan 20, 2016 · 7 comments
Closed

Add API to query OpenTracing version at runtime #33

bcronin opened this issue Jan 20, 2016 · 7 comments
Milestone

Comments

@bcronin
Copy link

bcronin commented Jan 20, 2016

On some platforms, like the browser, the binding between the API and a particular tracing implementation may only occur at runtime. For future-proofing it would be good, from the start, to expose a well-defined way for the implementation (or any code for that matter) to be able to check the OpenTracing version so compatibility constraints may be verified.

I'd propose the very simple API of:

  • Tracer.version() string returns the semantic version string of the OpenTracing API. In short, this means MAJOR.MINOR.PATCH - see semver.org for details. Note: this is the version of the OpenTracing API and is not related to the version of the tracing implementation being used.

I'm proposing a string return value matching a valid semantic version. I'm lightly arguing here that piggy-backing off the existing semantic versioning standard (while a bit more complicated to parse to handle all cases) is better than proposing, say, a simpler API that returns a 3-tuple of numbers (e.g. [major, minor, patch]); but I'm certainly open to being convinced otherwise :)

...

(Note: as a matter of practicality, I can see some argument for also exposing other details such as a standardized way to query the name of the Tracer implementation or other runtime details. However, for this particular issue, I'd like to keep the conversation targeted to whether the above API makes sense to everyone!)

@bhs
Copy link
Contributor

bhs commented Jan 21, 2016

Assuming we do this, it is small and should go in earlier rather than later... Assigning milestone accordingly.

@bhs bhs added this to the 1.0RC milestone Jan 21, 2016
@yurishkuro
Copy link
Member

I am not quite clear on how we expect users to use it.

In any case, to future proof it we can return some "metadata" / TracerInfo object that for now would only expose the api_version property.

@bcronin
Copy link
Author

bcronin commented Jan 21, 2016

I would advocate for some mechanism of exposing the version. As for the particular mechanism of delivering that version info (method vs object), I don't feel strongly as long as it's the same for 1.0 and 1.0+ or else the point is somewhat missed :)

My opinion is mostly driven by my experience working with public APIs, where I've found exposing version to be a best practice as forward/backward compatibility issues -- as much as they're not desired -- do inevitably arise in the imperfect world of practice.

As for a use case, I'm making this up off the top of my head, so I forgive me if this is not the best example:

  • Imagine I'm writing a jQuery plugin for making lots of XHRs via the public Google APIs, in JavaScript
  • I want to keep my script small so it checks for the Tracer global rather than including a copy of OpenTracing in my script
  • Per the last bullet, I tell my users to include opentracing as a separate <script> in their page
  • And the here's forward compatibility part: my library uses a feature from OpenTracing 1.1, say, Tracer.createTraceFromExtendedContext() (or some other currently imaginary method) so I tell them to include v1.1.0 or higher along with my library

The crux of my argument is it's lot cleaner for me (the implementor of that jQuery library) to add a fail-fast check against Tracer.version() in my library initialization than it is to rely on more specialized, platform-specific reflection / inspection abilities to see if createTraceFromExtendedContext() actually exists. (Also, more subtly, if there's an accidental semantic difference between version X and version Y, checking for presence of an API is not sufficient.) It's also a lot cleaner than waiting to die on the first call to the non-existent function if someone included 1.0.

It also affords me an opportunity to polyfill a createTraceFromExtendedContext if say I want to be compatible with some poorly-written-but-useful library that requires OpenTracing 1.0 (since it binds to it directly via a copy or does something annoying like overwrites the Tracer global with its own copy of Tracer...neither of which it really should have done, but did anyway).

All of this is of course not an issue in any configuration where all the binding can be done purely statically and not at run time. Note it's also a little hard to make a rock solid case for this since it's very much about future-proofing, not addressing an existing limitation or problem; such is the joy of forward-compatibility concerns :)

Hope that makes sense!

@yurishkuro
Copy link
Member

@bcronin thanks for the background, quite useful. I would still suggest an object with api_version method, to allow easier extension in the future, rather than being stuck with a top-level method on a tracer. E.g. the TracerInfo object may expose other methods describing other capabilities of the tracer, like the max number of tags per span (just a random example). Ideally, the compatibility checks you described above would be done by querying TracerInfo for certain features, which seems more robust than querying for API version, but I definitely do not want to design that kind of metadata now.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Jan 22, 2016 via email

@bhs
Copy link
Contributor

bhs commented Jan 26, 2016

(I also like the idea of using this to describe OpenTracing implementation support for certain perhaps-optional capabilities... examples might be formatted logging facilities or perhaps the "trace attributes" / "baggage" feature.)

@bcronin
Copy link
Author

bcronin commented Feb 12, 2016

I'm closing it per the discussion in opentracing/opentracing-python#16. I'm closing it as I originally opened it, but if anyone else feels strongly about this one, please just go ahead and reopen the issue!

@bcronin bcronin closed this as completed Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants