Introduce Observer and SpanObserver #94

yurishkuro · 2017-02-17T18:50:16Z

Intended to add observer to emit RPC metrics.

@black-adder @badiib

coveralls · 2017-02-17T18:55:35Z

Coverage decreased (-0.5%) to 81.324% when pulling 789b3a2 on observer into fe7c905 on master.

coveralls · 2017-02-17T18:55:35Z

Coverage decreased (-0.5%) to 81.324% when pulling 789b3a2 on observer into fe7c905 on master.

oibe · 2017-02-17T19:03:06Z

observer.go

+	for _, obs := range o.observers {
+		obs.OnSetTag(key, value)
+	}
+


nit: extra line here.

oibe · 2017-02-17T19:05:12Z

Why do we have a method like "OnSetTag", but not something like "OnSetLog"?

black-adder

@oibe the current use case for the observer is for metrics which needs tags, operationName, and startime to build the metrics name. If in the future we need further functionality for an observer, we can add OnSetLog.

LGTM, I'm still not completely sure why observer trumps decorator in this case but this looks good.

black-adder · 2017-02-17T19:26:02Z

observer.go

+	OnFinish(options opentracing.FinishOptions)
+}
+
+// observer is a disatcher to other observers


black-adder · 2017-02-17T20:17:55Z

observer.go

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


shouldn't this implement SpanObserver funcs?

black-adder · 2017-02-17T20:20:20Z

observer.go

+		spanObs := obs.OnStartSpan(operationName, options)
+		if spanObs != nil {
+			if spanObservers == nil {
+				spanObservers = make([]SpanObserver, 0, len(o.observers))


is this what we want? what if spanObs is nil for 1 out of len(o.observers)? we allocate more mem than needed.

do you have a better suggestion?

can't we get rid of L40:L42?

When you use append() the first capacity of the slice is 2, then it's doubled when it needs to grow. So it's quite possible that in your approach (a) even more memory is allocated than needed, and what's worse (b) potentially more than one memory allocation happens should the slice needs to be extended.

In my approach you get no more than one allocation, and the memory (a pointer) is only wasted if some observers are not interested in the span. I don't think this memory waste is that critical, the service would need to keep a lot of spans in-flight for this to matter

Aren't you tired of always being right?

yurishkuro · 2017-02-17T20:36:00Z

@oibe we can add onSetLog if/when this becomes necessary. Right now there's no use case for it so I am not adding it. Also, logs API is quite verbose, it's not clear what the callback should look like.

coveralls · 2017-02-17T20:46:32Z

Coverage decreased (-0.4%) to 81.449% when pulling ea99d0f on observer into fe7c905 on master.

coveralls · 2017-02-17T20:46:32Z

Coverage decreased (-0.4%) to 81.449% when pulling ea99d0f on observer into fe7c905 on master.

badiib

LGTM, the point i made does not apply to this diff directly.

badiib · 2017-02-20T16:10:59Z

tracer.go

-	startTime := options.StartTime
-	if startTime.IsZero() {
-		startTime = t.timeNow()
+	if options.StartTime.IsZero() {


why test for this? If we are testing that FinishTime is zero, and StartTime is also zero, the failure of a startTime to be set will be apparent in the UI, is a visible (and ugly) bug.

A span must have a start time. But options.startTime is an optional parameter, we do not expect most users to set it.

yurishkuro · 2017-02-25T16:55:19Z

I'm still not completely sure why observer trumps decorator in this case but this looks good.

@black-adder cf. #103 (rpcmetrics/observer.go). There is not a lot of difference with the decorator, but

there is no need for delegation to the real span, which means less overhead across the board
observer does not need to unwrap start options into a struct, which is a duplicated work. In particular, it does not need to make additional calls to time.Now()

coveralls · 2017-02-28T03:13:06Z

Coverage increased (+0.4%) to 82.244% when pulling e75f4bf on observer into fe7c905 on master.

coveralls · 2017-02-28T03:13:06Z

Coverage increased (+0.4%) to 82.244% when pulling e75f4bf on observer into fe7c905 on master.

Metrics are useful to gain insights in a distributed application. But there can be a lot of metrics in different domains. Adding such metrics to any one (client) tracer breaks cross platform compatibility. This commit adds a new observer and span observer API which defines a standard interface to add such metrics. To expose the metrics, one would have to implement the observer interface. The (client) tracers would just have to install callbacks for methods such as StartSpan, SetOperationName, SetTag and Finish. The registered callbacks would then be called on the span events if an observer is created. This is based on the work done by Yuri here : jaegertracing/jaeger-client-go#94 Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Hemant Kumar <[email protected]>

coveralls · 2017-03-02T16:21:18Z

Coverage increased (+0.09%) to 82.085% when pulling bbfe8c1 on observer into 2505ffc on master.

coveralls · 2017-03-02T16:21:27Z

Coverage increased (+0.09%) to 82.085% when pulling bbfe8c1 on observer into 2505ffc on master.

oibe · 2017-03-02T18:29:11Z

observer_test.go

+
+type testObserver struct{}
+
+type testSpanObserver struct {


Is there a reason to create a test struct vs using "mockery"? Is it just because this is simpler?

for the clients, we want them as lightweight as possible, the fewer deps the better

@oibe is there a reason to use mockery instead of creating a struct?

Metrics are useful to gain insights in a distributed application. But there can be a lot of metrics in different domains. Adding such metrics from one domain (metrics exporter pkg) into zipkin is not good for cross platform compatibility. This commit adds a new observer and span observer API which defines a standard interface to add such metrics. To expose the metrics, one would have to implement the observer interface in the metrics exporter. zipkin will need to implement this interface and install callbacks for methods such as StartSpan, SetOperationName, SetTag and Finish. The registered callbacks would then be called on the span events if an observer is created. This is based on the work done by Yuri here : jaegertracing/jaeger-client-go#94 Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Hemant Kumar <[email protected]>

oibe reviewed Feb 17, 2017

View reviewed changes

observer.go Outdated

for _, obs := range o.observers {

obs.OnSetTag(key, value)

}

Copy link

oibe Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line here.

black-adder reviewed Feb 17, 2017

View reviewed changes

badiib approved these changes Feb 20, 2017

View reviewed changes

yurishkuro mentioned this pull request Feb 21, 2017

Add support for perfevents and an Observer API opentracing/opentracing-go#135

Closed

bhs mentioned this pull request Feb 21, 2017

Consider adding an Observer API opentracing/opentracing-go#136

Open

hkshaw1990 mentioned this pull request Feb 24, 2017

Monitoring platform events with zipkin-go and opentracing openzipkin-contrib/zipkin-go-opentracing#50

Merged

yurishkuro mentioned this pull request Feb 25, 2017

Add RPC metrics emitter as Observer/SpanObserver #103

Merged

yurishkuro changed the title ~~RFC: Introduce Observer and SpanObserver~~ Introduce Observer and SpanObserver Mar 1, 2017

Yuri Shkuro added 7 commits March 1, 2017 17:15

Introduce (Span) Observer API

500898c

Implement dispatchers

c430c12

Typo

259a0a0

add tests and license header

efa5448

Compare struct

bfb4cce

Add config option

ca05f85

Update changelog

bbfe8c1

yurishkuro force-pushed the observer branch from e5cd8ae to bbfe8c1 Compare March 2, 2017 16:11

black-adder approved these changes Mar 2, 2017

View reviewed changes

oibe reviewed Mar 2, 2017

View reviewed changes

yurishkuro merged commit 0505b4d into master Mar 2, 2017

yurishkuro deleted the observer branch May 5, 2021 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Observer and SpanObserver #94

Introduce Observer and SpanObserver #94

yurishkuro commented Feb 17, 2017 •

edited

Loading

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017

oibe Feb 17, 2017

oibe commented Feb 17, 2017

black-adder left a comment

black-adder Feb 17, 2017

black-adder Feb 17, 2017

yurishkuro Feb 17, 2017

black-adder Feb 17, 2017

yurishkuro Feb 17, 2017

black-adder Feb 17, 2017

yurishkuro Feb 17, 2017

black-adder Feb 17, 2017

yurishkuro commented Feb 17, 2017

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017

badiib left a comment

badiib Feb 20, 2017

yurishkuro Feb 20, 2017

yurishkuro commented Feb 25, 2017

coveralls commented Feb 28, 2017

coveralls commented Feb 28, 2017

coveralls commented Mar 2, 2017

coveralls commented Mar 2, 2017

oibe Mar 2, 2017

black-adder Mar 2, 2017

yurishkuro Mar 2, 2017

Introduce Observer and SpanObserver #94

Introduce Observer and SpanObserver #94

Conversation

yurishkuro commented Feb 17, 2017 • edited Loading

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017

Choose a reason for hiding this comment

oibe commented Feb 17, 2017

black-adder 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

yurishkuro commented Feb 17, 2017

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017

badiib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 25, 2017

coveralls commented Feb 28, 2017

coveralls commented Feb 28, 2017

coveralls commented Mar 2, 2017

coveralls commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 17, 2017 •

edited

Loading