Add span context to OnStartSpan #159

hkshaw1990 · 2017-06-12T09:25:38Z

Some observers will need to call span functions from within the observed span events.

CLAassistant · 2017-06-12T09:25:45Z

All committers have signed the CLA.

coveralls · 2017-06-12T09:29:04Z

Coverage remained the same at 85.355% when pulling 8bd55d8 on hkshaw1990:observer-with-spancontext into d649bde on uber:master.

yurishkuro · 2017-06-12T16:01:36Z

observer.go

@@ -24,7 +24,7 @@ import opentracing "github.com/opentracing/opentracing-go"

 // Observer can be registered with the Tracer to receive notifications about new Spans.
 type Observer interface {
-	OnStartSpan(operationName string, options opentracing.StartSpanOptions) SpanObserver
+	OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) SpanObserver


There's a problem with changing this interface - it is a breaking change and would require a major version upgrade. Given opentracing-contrib/go-observer#1, what if we leave this unchanged (document as deprecated) and instead add support for "options.ContribObserver()"?

We will support both until the next major release of jaeger-client, at which point we can consolidate into one AP.

@yurishkuro Agreed and thanks for the suggestion.
Have added a commit which reverts this and adds ContribObserver.

yurishkuro · 2017-06-21T17:51:03Z

config/options.go

@@ -24,6 +24,8 @@ import (
 	"github.com/uber/jaeger-lib/metrics"

 	"github.com/uber/jaeger-client-go"
+
+	otobserver "github.com/opentracing-contrib/go-observer"


I would prefer not to have an explicit dependency on this package, but instead just re-define its interfaces here. Due to duck typing it should still be compatible, but Jaeger will be isolated from the potential API changes in the contrib package.

Ok. I tried this with the latest commits I added here. And, it shows some compatibility issues, i.e., if I initialize a new observer based on go-observer and try to assign this observer with ContribObserver() in cfg.New(), I get a compatibility error :
have OnStartSpan(opentracing.Span, string, opentracing.StartSpanOptions) (otobserver.SpanObserver, bool)
want OnStartSpan(opentracing.Span, string, opentracing.StartSpanOptions) (jaeger.ContribSpanObserver, bool)

yurishkuro · 2017-06-21T17:52:30Z

contrib_observer.go

@@ -0,0 +1,90 @@
+// Copyright (c) 2017 Uber Technologies, Inc.
+//           (c) 2017 IBM Corp.


When you sign the CLA it assigns copyright to Uber, so please don't include this here, because it restricts our ability to manage licensing of Jaeger packages.

Didn't know that. Updated.

yurishkuro · 2017-06-21T17:55:57Z

config/options.go

@@ -64,8 +66,18 @@ func Reporter(reporter jaeger.Reporter) Option {

 // Observer can be registered with the Tracer to receive notifications about new Spans.
 func Observer(observer jaeger.Observer) Option {
+	old := jaeger.OldObserver{Obs: observer}


it's not good having OldObserver public. There are two ways we can handle this:

duplicate ContribObserver option on the Tracer and do the wrapping there and only there, thus making oldObserver private

or define an internal/observer package and declare a public type there - by convention people should not be depending on APIs under internal

I went with the 1st option, since, the 2nd option will still make old observer public.

yurishkuro · 2017-06-21T17:58:51Z

span.go

@@ -28,6 +28,8 @@ import (
 	"github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/ext"
 	"github.com/opentracing/opentracing-go/log"
+
+	otobserver "github.com/opentracing-contrib/go-observer"


undesired dependency

yurishkuro · 2017-06-21T17:59:35Z

span.go

@@ -63,7 +65,7 @@ type Span struct {
 	// references for this span
 	references []Reference

-	observer SpanObserver
+	contribObserver otobserver.SpanObserver


I think you can keep the name observer

Ok, changed this.

yurishkuro · 2017-06-22T16:17:48Z

Ok. I tried this with the latest commits I added here. And, it shows some compatibility issues

Hm, yes, that's unfortunate, duck typing doesn't work when you have an interface returning an interface.

But to me a dependency on a contrib package is a bigger issue, for semver compatibility reasons, especially on a contrib pkg that is brand new and not battle tested. We may need to do a compromise. The duck typing will still work on the SpanObserver, but for the main Observer the end user would have to write a small wrapper that implements jaeger.ContribObserver interface. I don't think it's a huge burden because it's just one function and the end user needs to have some custom code when initializing Jaeger tracer anyway.

So if you have a perf events related contrib.Observer, you would do something like this when passing it to Jaeger

type jaegerObserver struct {
    delegate *perfevents.Observer
}

// implement jaeger.ContribObserver
func (jo *jaegerObserver) OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) (jaeger.ContribSpanObserver, bool) {
    return delegate.OnStartSpanRaw(sp, opName, ops)
}

as long as delegate.OnStartSpanRaw(sp, opName, ops) returns the raw *perfevents.SpanObserver (instead of the interface), the duck typing will work.

It's a bit of a dance, we can clean it up once the contrib.Observer gets more battle tested and the repo starts releasing semver.

hkshaw1990 · 2017-06-22T17:26:43Z

as long as delegate.OnStartSpanRaw(sp, opName, ops) returns the raw *perfevents.SpanObserver (instead of the interface), the duck typing will work.

@yurishkuro alright, seems ok. We can have that bit of the initialization code.

Any other comments on the added commits?

yurishkuro · 2017-06-22T19:05:46Z

contrib_observer.go

+	//         var sp opentracing.Span
+	//         sso := opentracing.StartSpanOptions{}
+	//         spanObserver, ok := Observer.OnStartSpan(span, opName, sso);
+	//         if ok {


this should be (idiomatic Go)

if spanObserver, ok := Observer.OnStartSpan(span, opName, sso); ok { // we have a valid SpanObserver }

Changed this.

yurishkuro · 2017-06-22T19:07:06Z

contrib_observer.go

+}
+
+// ContribSpanObserver is created by the Observer and receives notifications
+// about other Span events.


might add: It is meant to match http://...contrib-observer, but via duck typing, without directly importing the contrib package.

Added this comment.

yurishkuro · 2017-06-22T19:09:27Z

contrib_observer.go

+}
+
+// contribSpanObserver is a dispatcher to other span observers
+type contribSpanObserver struct {


s/contribSpanObserver/compositeSpanObserver/ (we use "composite" in many places). compositeObserver at L63 as well.

I assume the nearly identical old dispatcher should be deleted?

Yeah, changed the naming convention from contrib to composite and deleted the old dispatcher.

yurishkuro · 2017-06-22T19:12:35Z

tracer.go

@@ -59,7 +59,7 @@ type tracer struct {
 	injectors  map[interface{}]Injector
 	extractors map[interface{}]Extractor

-	observer observer
+	contribObserver contribObserver


nit: variable name should remain observer

yurishkuro · 2017-06-22T19:14:32Z

contrib_observer.go

+
+func (o contribObserver) OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) (ContribSpanObserver, bool) {
+	var spanObservers []ContribSpanObserver
+	for _, obs := range o.observers {


we should add a TODO to optimize this for the case when ==1 span observer is created - it can be returned directly, without allocating the slice.

Didn't entirely understand this part. Can you please elaborate?

yurishkuro · 2017-06-22T19:18:10Z

contrib_observer.go

+		}
+	}
+	if len(spanObservers) == 0 {
+		return noopContribSpanObserver, false


tracer has a direct dependency on this struct, so the function signature does not need to match the API. Specifically, the boolean value is meaningless because we always return a valid (albeit noop) span observer.

Right. Nice suggestion. Updated in the latest commits.

yurishkuro · 2017-06-22T19:18:33Z

tracer.go

@@ -233,7 +233,7 @@ func (t *tracer) startSpanWithOptions(

 	sp := t.newSpan()
 	sp.context = ctx
-	sp.observer = t.observer.OnStartSpan(operationName, options)
+	sp.observer, _ = t.contribObserver.OnStartSpan(sp, operationName, options)


per comment above, there's no need for the second return value.

omeid · 2017-06-23T05:30:21Z

Couldn't the span be just passed to a normal observer? I know this is breaking change but observer without access to span is not much of use really.
My use case is sending span logs to a different backend.

yurishkuro · 2017-06-23T05:34:44Z

@omeid the "normal" observer is the interface we had before the new one in contrib, and it did not accept the Span. If you need a span you can just implement the new ContribObserver

omeid · 2017-06-23T05:39:41Z

But this doesn't seem to be merged in, right? Also, how do I get a hang of the logs from a span? They seem to be unexported. :(

coveralls · 2017-07-04T10:06:40Z

Coverage decreased (-0.9%) to 84.454% when pulling b2ac245 on hkshaw1990:observer-with-spancontext into d649bde on uber:master.

yurishkuro

minor clean-up needed, otherwise lgtm

Note about pointers:

the compositeObserver is an addressable field in the tracer, it does not need to be declared as a pointer, but there's no reason to have its functions not use the pointer to avoid copying
the compositeSpanObserver instances should always be by pointer. When an instance of composite span observer is returned, the return type is an interface, so heap allocation will happen anyway, better to do that explicitly and avoid copying when calling functions on it.

yurishkuro · 2017-07-04T16:42:49Z

contrib_observer.go

+
+// noopCompositeSpanObserver is used when there are no observers registered
+// on the Tracer or none of them returns span observers from OnStartSpan.
+var noopCompositeSpanObserver = compositeSpanObserver{}


take a pointer: = &compositeSpanObserver{}

yurishkuro · 2017-07-04T16:42:57Z

config/config.go

@@ -147,10 +147,14 @@ func (c Configuration) New(
 		jaeger.TracerOptions.ZipkinSharedRPCSpan(opts.zipkinSharedRPCSpan),
 	}

-	for _, obs := range opts.observers {
+	for _,obs := range opts.observers {


yurishkuro · 2017-07-04T16:43:56Z

contrib_observer.go

+	obs Observer
+}
+
+func (o oldObserver) OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) (ContribSpanObserver, bool) {


func (o *oldObserver)

yurishkuro · 2017-07-04T16:44:26Z

contrib_observer.go

+	return compositeSpanObserver{observers: spanObservers}
+}
+
+func (o compositeSpanObserver) OnSetOperationName(operationName string) {


func (o *compositeSpanObserver)

yurishkuro · 2017-07-04T16:44:52Z

contrib_observer.go

+	}
+}
+
+func (o compositeSpanObserver) OnSetTag(key string, value interface{}) {


func (o *compositeSpanObserver)

yurishkuro · 2017-07-04T16:49:22Z

observer.go

@@ -22,12 +22,16 @@ package jaeger

 import opentracing "github.com/opentracing/opentracing-go"

-// Observer can be registered with the Tracer to receive notifications about new Spans.
+// Observer can be registered with the Tracer to receive notifications about
+// new Spans. Note that this interface is now Deprecated, see:


See the recommended format for deprecation notice https://rakyll.org/deprecated/

[blank line] Deprecated: use jaeger.ContribObserver instead.

Right. Made this and all the above changes except one.

yurishkuro · 2017-07-04T16:51:45Z

observer.go

-// SpanObserver is created by the Observer and receives notifications about other Span events.
+// SpanObserver is created by the Observer and receives notifications about
+// other Span events. Note that this interface is now Deprecated, see:
+// github.com/opentracing-contrib/go-observer.


[blank line] Deprecated: use jaeger.ContribSpanObserver instead.

yurishkuro · 2017-07-04T16:52:20Z

observer.go

@@ -39,50 +43,27 @@ type observer struct {
 	observers []Observer
 }


Is this still needed? I thought compositeObserver above would be used instead,

The observer is probably needed since, it implements the Observer interface. If we remove this struct observer, the function OnStartSpan(operationName string, options opentracing.StartSpanOptions) i.e., without the span argument, can't be defined which in turn will affect the existing external observers, right? Is there any other way we can replace this?

when old-style observers are passed to tracer or config via option, they are immediately converted to the contrib-style, thus we never need to use the old style observer, it is fully replaced by the new compositeObserver

yurishkuro · 2017-07-04T16:55:10Z

contrib_observer.go

+}
+
+// compositeObserver is a dispatcher to other observers
+type compositeObserver struct {


I prefer to move this to observer.go (replace the old type). In the next major we can remove the contrib file/types.

Regarding moving this and replacing the old type, see below.

replied below. The main reason to move it to observer.go is to reduce the size of the diff.

yurishkuro · 2017-07-04T16:59:30Z

contrib_observer.go

+	o.observers = append(o.observers, contribObserver)
+}
+
+func (o compositeObserver) OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) ContribSpanObserver {


func (o *compositeObserver)

yurishkuro · 2017-07-06T14:32:51Z

please update your master and rebase

Some of the observers will need to call span functions from within the observed span events.

Reverts the previous commit (add span context to OnStartSpan) since, it will break the existing observers. Adds another observer contribObserver instead, implementing the interface github.com/opentracing-contrib/go-observer.

Also renames the contribObserver -> observer in the Span struct.

Change the global interfaces back to contrib observer. Only the private struct implementations remain as composite*. This patch also fixes a test error by changing spanObserver to compositeSpanObserver.

yurishkuro · 2017-07-06T16:44:09Z

config/config.go

@@ -147,12 +147,16 @@ func (c Configuration) New(
 		jaeger.TracerOptions.ZipkinSharedRPCSpan(opts.zipkinSharedRPCSpan),
 	}

+	for _, tag := range opts.tags {
+		tracerOptions = append(tracerOptions, jaeger.TracerOptions.Tag(tag.Key, tag.Value))
+	}


hm, merge issue. You need to define the official repo as eg upstream and

git checkout -b master git pull upstream master git checkout -b observer-with-spancontext git rebase master

your master should be == upstream master

Yeah, did the exact same thing, but I guess this crept on while resolving some conflicts. Will fix it.

yurishkuro · 2017-07-08T18:17:00Z

tracer_options.go

+
+	return func(tracer *Tracer) {
+		tracer.observer.append(&old)
+	}


just delegate (DRY)

func (tracerOptions) Observer(observer Observer) TracerOption { return ContribObserver(&oldObserver{obs: observer}) }

Ok, but delegation will require scope resolution here, since, ContribObserver is also an interface and a tracerOptions function. Are you ok with having this (asking this because it doesn't conform to the current convention for other functions in tracer_options.go)?

func (t *tracerOptions) Observer(observer Observer) TracerOption { return t.ContribObserver(&oldObserver{obs: observer}) }

sorry, yes delegate to scoped tracerOptions.ContribObserver function, so there's no naming conflicts.

Thanks, updated.

yurishkuro · 2017-07-08T18:19:15Z

contrib_observer.go

+}
+
+func (o *oldObserver) OnStartSpan(sp opentracing.Span, operationName string, options opentracing.StartSpanOptions) (ContribSpanObserver, bool) {
+	return o.obs.OnStartSpan(operationName, options), true


spanObserver := o.obs.OnStartSpan(operationName, options) return spanObserver, spanObserver != nil

Made the change.

yurishkuro · 2017-07-08T18:20:19Z

contrib_observer.go

+
+// noopCompositeSpanObserver is used when there are no observers registered
+// on the Tracer or none of them returns span observers from OnStartSpan.
+var noopCompositeSpanObserver = &compositeSpanObserver{}


nit: s/noopCompositeSpanObserver/noopSpanObserver/ - it is not relevant that it is "composite", just that it's "no-op"

For some reason, I am not able to reply to the previous comment, so, I will add my reply here:

replied below. The main reason to move it to observer.go is to reduce the size of the diff.

Got it. So, it does reduce the diff when the compositeObserver struct and its functions are moved to observer.go. But, to be clear, did you ask me to move all of compositeObserver and compositeSpanObserver code to observer.go leaving only the ContribObserver and ContribSpanObserver interfaces there?

yurishkuro · 2017-07-08T18:23:27Z

observer.go

@@ -39,50 +43,27 @@ type observer struct {
 	observers []Observer
 }


when old-style observers are passed to tracer or config via option, they are immediately converted to the contrib-style, thus we never need to use the old style observer, it is fully replaced by the new compositeObserver

yurishkuro · 2017-07-08T18:25:25Z

contrib_observer.go

+}
+
+// compositeObserver is a dispatcher to other observers
+type compositeObserver struct {


replied below. The main reason to move it to observer.go is to reduce the size of the diff.

coveralls · 2017-07-13T17:22:17Z

Coverage decreased (-0.1%) to 85.093% when pulling 59e4a99 on hkshaw1990:observer-with-spancontext into c0cb42c on uber:master.

coveralls · 2017-07-13T17:32:15Z

Coverage decreased (-0.1%) to 85.093% when pulling b06b8a1 on hkshaw1990:observer-with-spancontext into c0cb42c on uber:master.

yurishkuro · 2017-07-13T19:46:20Z

Thanks for your patience, @hkshaw1990 .

yurishkuro reviewed Jun 12, 2017

View reviewed changes

yurishkuro reviewed Jun 21, 2017

View reviewed changes

yurishkuro reviewed Jun 22, 2017

View reviewed changes

black-adder mentioned this pull request Jun 24, 2017

More Comperhensive Reporters #172

Closed

yurishkuro reviewed Jul 4, 2017

View reviewed changes

hkshaw1990 added 12 commits July 6, 2017 20:22

Add span context to OnStartSpan

9806da7

Some of the observers will need to call span functions from within the observed span events.

Introduce contribObserver and contribSpanObserver

19e8a3d

Reverts the previous commit (add span context to OnStartSpan) since, it will break the existing observers. Adds another observer contribObserver instead, implementing the interface github.com/opentracing-contrib/go-observer.

Wrap the old observer to contrib observer

702850d

Remove copyright

8217148

Redefine go-observer interface as ContribObserver

cd19a93

Add bool according to the API changes in go-observer

eaa3e43

Also renames the contribObserver -> observer in the Span struct.

Add comments to contrib_observer.go

f21c9cd

Rephrase comments

4cd454f

Rename contrib* to composite* for observer api

3f0685b

Remove bool from compositeObserver's OnStartSpan

5d2dbd3

Retire the old observer dispatcher and related callbacks

02b63d7

Rename the global interfaces for the new observer

2a32e77

Change the global interfaces back to contrib observer. Only the private struct implementations remain as composite*. This patch also fixes a test error by changing spanObserver to compositeSpanObserver.

hkshaw1990 added 3 commits July 6, 2017 20:38

Use pointers for compositeObserver and compositeSpanObserver methods

22fb0a2

Fix Formatting issues

a06ccab

Fix the tracer argument type

cb923e2

hkshaw1990 force-pushed the observer-with-spancontext branch from f8643e5 to cb923e2 Compare July 6, 2017 15:15

yurishkuro reviewed Jul 6, 2017

View reviewed changes

Resolve a merge issue by moving the tags part in config/config.go

38c6fe4

yurishkuro reviewed Jul 8, 2017

View reviewed changes

hkshaw1990 added 3 commits July 10, 2017 17:33

Remove the old dispatcher and move the new dispatcher to observer.go

d2ddb67

Delegate from Observer to ContribObserver for tracer options

9ab053e

Fix a type assertion causing build failure in observer_test.go

59e4a99

Fix whitespace issue

b06b8a1

yurishkuro approved these changes Jul 13, 2017

View reviewed changes

yurishkuro merged commit feadba6 into jaegertracing:master Jul 13, 2017

yurishkuro mentioned this pull request Jul 29, 2017

Release 2.9.0 #180

Merged

		@@ -0,0 +1,90 @@
		// Copyright (c) 2017 Uber Technologies, Inc.
		// (c) 2017 IBM Corp.

		@@ -39,50 +43,27 @@ type observer struct {
		observers []Observer
		}

Add span context to OnStartSpan #159

Add span context to OnStartSpan #159

Conversation

hkshaw1990 commented Jun 12, 2017

CLAassistant commented Jun 12, 2017 • edited Loading

coveralls commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkshaw1990 Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jun 22, 2017

hkshaw1990 commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkshaw1990 Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkshaw1990 Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omeid commented Jun 23, 2017

yurishkuro commented Jun 23, 2017

omeid commented Jun 23, 2017

coveralls commented Jul 4, 2017

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkshaw1990 Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

yurishkuro commented Jul 13, 2017

CLAassistant commented Jun 12, 2017 •

edited

Loading

hkshaw1990 Jun 22, 2017 •

edited

Loading

yurishkuro Jun 21, 2017 •

edited

Loading

hkshaw1990 Jun 23, 2017 •

edited

Loading

hkshaw1990 Jun 23, 2017 •

edited

Loading

hkshaw1990 Jul 10, 2017 •

edited

Loading