-
Notifications
You must be signed in to change notification settings - Fork 316
add a OT-Go semver #51
Conversation
|
||
// ImplementationID is a simple, extensible struct that describes an | ||
// OpenTracing implementation. | ||
type ImplementationID struct { |
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.
(the only interesting part of this change)
This intentionally does not handle any notion of |
|
||
// Version may take any form, but SemVer (http://semver.org/) is strongly | ||
// encouraged. | ||
Version string |
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.
@bensigelman , this looks pretty good, but Go seems a bit like the exceptional case with regards to the utility of a version string (since everything in Go is basically statically bound). I'm curious what this might look like for Python. I'm specifically considering the case where the supported version of the OpenTracing standard is what's of interest rather than the underlying implementation version. E.g. if an arbitrary piece of Python code is handed a tracer
object, how can it (cleanly) check the supported version of OpenTracing on that object?
(By the way I believe I'm safe in assuming that in the above Version
is intended to represent the implementation version given the struct name and the "may take any form" comment.)
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 with @bcronin, it's the OpenTracing version that the original ticket was asking for. We said it would be best done via Capabilities API rather than a version. At the moment, however, there doesn't seem to be any aspects of the API that are optional and require introspection by the client code.
@bcronin @yurishkuro roger that... makes sense. I added a |
return standardtracer.New(NewTrivialRecorder(processName)) | ||
return standardtracer.New( | ||
NewTrivialRecorder(processName), | ||
&opentracing.ImplementationID{ |
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.
you're explicitly letting tracers return nil
here? Doesn't seem like a bad idea to force tracers to pick a name and version.
Per opentracing/opentracing-python#16, I'm pulling this back to be a semver for the Go API and nothing else. |
(withdrawing this per opentracing/opentracing.io#33) |
An RFC per opentracing/opentracing.io#33 (@bcronin)