Deprecate InitGlobalTracer in favor of SetGlobalTracer #128

Merged
merged 2 commits into from Dec 2, 2016

Projects

None yet

4 participants

@yurishkuro
Contributor
yurishkuro commented Nov 30, 2016 edited
  • Clarify the purpose of NoopTracer
This was referenced Nov 30, 2016
@bensigelman
Contributor

My only other thought would be to make the global tracer an exported var... unless we think the mutator (or accessor) will eventually do something more interesting?

@yurishkuro
Contributor

I'm fine with a var. But GlobalTracer isn't backwards compatible with the current method name. DefaultTracer is an option.

@bensigelman
Contributor

Meh, I thought about it some more and like what you have here. If for some reason we decide to do more than just set the (unexported) global var someday, this will be better.

@bensigelman
Contributor

(i.e., LGTM)

@kriskowal

Needs a change log entry. Recommend major version bump, or alternately alias Init*

@bensigelman
Contributor

@kriskowal it's already aliased.

+// not need to keep checking if the tracer instance is nil.
+//
+// For the same reason, the NoopTracer is the default "global" tracer
+// (see GlobalTracer and SetGlobalTracer functions).
@kriskowal
kriskowal Dec 1, 2016 Collaborator

Please also note that NoopTracer is not suitable for RPC frameworks because it does not propagate baggage.

@yurishkuro
yurishkuro Dec 2, 2016 Contributor

I added a warning below. But I don't think it's fair to say that it's not suitable for RPC frameworks - it is used in TChannel, for example. Even if we took a reverse approach and built tracing on top of distributed context propagation, there'd still be a case where API for context propagation would have a no-op implementation meaning "no context propagation", and you'd have the same problem.

@kriskowal
kriskowal Dec 2, 2016 Collaborator

Ok. It is perhaps too strong to claim that NoopTracer is suitable for RPC, since it would need to bring its own baggage propagation to the table like TChannel, just as it is too strong to claim that it is not suitable for RPC.

.travis.yml
@@ -1,7 +1,8 @@
language: go
go:
- - 1.5
@breerly
breerly Dec 1, 2016 edited

You can't drop support for 1.5 now that you've gone 1.0 - the customer expectation is that within 1.x, there are no breaking changes. Removing this would mean we wouldn't know if 1.5 was broken within the 1.x series.

// (etc) globals are noops.
-func InitGlobalTracer(tracer Tracer) {
+func SetGlobalTracer(tracer Tracer) {
@breerly
breerly Dec 1, 2016

Renaming this is a backwards breaking change. Suggest creating an alias instead, or incrementing the major version.

@breerly
breerly Dec 1, 2016

Looks like there is an alias.

@@ -25,3 +25,8 @@ func GlobalTracer() Tracer {
func StartSpan(operationName string, opts ...StartSpanOption) Span {
return globalTracer.StartSpan(operationName, opts...)
}
+
+// InitGlobalTracer is deprecated. Please use SetGlobalTracer.
+func InitGlobalTracer(tracer Tracer) {
@kriskowal
kriskowal Dec 1, 2016 Collaborator

found it, thanks @bensigelman

yurishkuro added some commits Nov 30, 2016
@yurishkuro yurishkuro Clarify the purpose of NoopTracer; rename InitGlobalTracer to SetGlob…
…alTracer
0e6b205
@yurishkuro yurishkuro Add warning about baggage; update changelog
e7527cd
@yurishkuro yurishkuro changed the title from Clarify the purpose of NoopTracer; rename InitGlobalTracer to Set… to Deprecate InitGlobalTracer in favor of SetGlobalTracer Dec 2, 2016
+// not need to keep checking if the tracer instance is nil.
+//
+// For the same reason, the NoopTracer is the default "global" tracer
+// (see GlobalTracer and SetGlobalTracer functions).
@kriskowal
kriskowal Dec 2, 2016 Collaborator

Ok. It is perhaps too strong to claim that NoopTracer is suitable for RPC, since it would need to bring its own baggage propagation to the table like TChannel, just as it is too strong to claim that it is not suitable for RPC.

@yurishkuro yurishkuro merged commit ac5446f into master Dec 2, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@RaduBerinde RaduBerinde added a commit to RaduBerinde/lightstep-tracer-go that referenced this pull request Dec 8, 2016
@RaduBerinde RaduBerinde README:md: update deprecated call in example
`InitGlobalTracer` was deprecated for `SetGlobalTracer` in opentracing/opentracing-go#128
6b049ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment