Initial observer API #1
Conversation
Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Hemant Kumar <[email protected]>
observer.go
Outdated
| // ... | ||
| // } | ||
| // OnStartSpan function needs to be defined for a package exporting | ||
| // metrics as well. |
There was a problem hiding this comment.
should this comment be here? it's a bit confusing
README.md
Outdated
| As noble as our thoughts might be in fetching additional metrics (other than | ||
| latency) for a span using an observer, there are some overhead costs. Not all | ||
| observers need to observe all the span events, in which case, we are keeping | ||
| a (or, some) handler function(s) empty. In effect, we will still call |
There was a problem hiding this comment.
@yurishkuro the new commit rephrased this and removed the other confusing comments.
observer.go
Outdated
| // SpanObserver is created by the Observer and receives notifications about | ||
| // other Span events. | ||
| // Client tracers should define these functions for each of the span operations | ||
| // which should call the registered (observer) callbacks. |
There was a problem hiding this comment.
don't think this sentence is useful.
|
Given all of the previous discussion, and the limitations we currently have due to the lack of a |
README.md
Outdated
|
|
||
| The `otobserver` package provides an API. The (client) tracers (if they want | ||
| to implement the observer interface) can implement this interface. To do | ||
| that, first fetch the package using go get : |
There was a problem hiding this comment.
this description seems a bit off. The API provided by the tracers would be something like a functional option to tracer constructor that passes an Observer. The actual observer API is implemented by a 3rd party, not by the tracer itself.
There was a problem hiding this comment.
Yeah. Rephrased this in the new commit.
README.md
Outdated
|
|
||
| and then, define the required span event callbacks. These registered | ||
| callbacks would then be called on span events if an observer is created. | ||
| Multiple observers can be registered for a given distributed tracer. Have a |
There was a problem hiding this comment.
nit: Tracers may allow registering multiple observers (i.e. s/can/may/)
README.md
Outdated
| As noble as our thoughts might be in fetching additional metrics (other than | ||
| latency) for a span using an observer, there are some overhead costs. Not all | ||
| observers need to observe all the span events, in which case, we may have | ||
| to keep some callback function(s) empty. In effect, we will still call these |
observer.go
Outdated
| @@ -0,0 +1,36 @@ | |||
| // Copyright (c) 2017 Hemant Kumar, IBM Corp. | |||
| // (c) 2017 Yuri Shkuro, Uber Technologies, Inc. | |||
There was a problem hiding this comment.
@yurishkuro Removed that from this file and added a copyright for opentracing-contrib in the LICENSE file (following the convention in other opentracing-contrib projects).
Previous discussion and PR : opentracing/opentracing-go#135
Issue in opentracing-go : opentracing/opentracing-go#136