WIP: Named tracer prototype #227

rghetia · 2019-10-21T13:56:51Z

This is a prototype for named tracer. What is in here?

Global Provider Register (instead of Tracer Registry). This may not be required.
SDK provides NewProvider method to create instance of Provider. Multiple provider instance can be created. This can help to parallelized testing.
- It accepts optional parameter to associate exporters and configuration (sampler and other parameters) during instance creation.
- exporters and configuration can also be applied at a later time.
Some Tests are updated to created its own provider. Remove reference to global tracer.
- few tests still uses Global Tracer.

jmacd

Looks good, especially nice to see all the globals disappear from the SDK.
I have a few thoughts about how naming might improve, in light of the proposal in #216, but don't want to block this from merging.

jmacd · 2019-10-21T17:38:03Z

sdk/trace/benchmark_test.go

@@ -144,6 +145,7 @@ func BenchmarkSpanWithAttributes_all_2x(b *testing.B) {
 }

 func BenchmarkTraceID_DotString(b *testing.B) {
+	getTracer(b, "Benchmark traceID to string")


Is this needed?

yes. Without this tracer doesn't have a reference to the provider which has the config.

It just doesn't look like the tracer is used in this benchmark. No big deal.

I was looking in the wrong place. Yes, it is not needed. I have removed it.

jmacd · 2019-10-21T17:38:08Z

sdk/trace/benchmark_test.go

@@ -157,6 +159,7 @@ func BenchmarkTraceID_DotString(b *testing.B) {
 }

 func BenchmarkSpanID_DotString(b *testing.B) {
+	getTracer(b, "Benchmark spanID to string")


jmacd · 2019-10-21T17:42:18Z

api/trace/api.go

+	// GetTracer creates a named tracer that implements Tracer interface.
+	// If name is an empty string then default name from the manager
+	// is used.
+	GetTracer(name string) Tracer


I wonder if this should just be Get(name string) Tracer.

I'll fix this in another PR when I move the provider interface to global package.

jmacd · 2019-10-21T17:45:25Z

My concern about naming is that

   tracer := trace.GlobalProvider().GetTracer(name)

is fairly long. Renaming GetTracer() to Get() may help. In #216 I've proposed that globals move out of the core packages, so then I could imagine a package named global that would give us calls like:

  tracer := global.GetTracer(name)

the Init() method proposed in OTEP 0005 would take both a metric and a trace provider, but the caller doesn't need to write "provider" when accessing the globals.

akvanhar · 2019-10-21T17:56:59Z

example/http-stackdriver/client/client.go

 }

 func main() {
 	initTracer()
+	tr := trace.GlobalProvider().GetTracer("stackdriver/example/server")


Should this be /client?

yes. changed.

akvanhar · 2019-10-21T18:02:07Z

plugin/othttp/server_example_test.go

-	// For the example, use sdktrace.AlwaysSample sampler to sample all traces.
-	// In a production application, use sdktrace.ProbabilitySampler with a desired probability.
-	sdktrace.ApplyConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()})
+	//import sdktrace "go.opentelemetry.io/sdk/trace"


This comment doesn't make sense anymore.

akvanhar · 2019-10-21T18:02:35Z

sdk/trace/batch_span_processor_test.go

@@ -181,16 +178,16 @@ func createAndRegisterBatchSP(t *testing.T, option testOption, te *testBatchExpo
 	if ssp == nil {
 		t.Errorf("%s: Error creating new instance of BatchSpanProcessor, error: %v\n", option.name, err)
 	}
-	sdktrace.RegisterSpanProcessor(ssp)
+	//sdktrace.RegisterSpanProcessor(ssp)


delete this comment

akvanhar · 2019-10-21T18:05:26Z

sdk/trace/provider.go

+}
+
+// UnregisterSpanProcessor removes from the list of SpanProcessors the SpanProcessor that was
+// registered with the given name.


This comment is quite stuttery.
Suggestion:
UnregisterSpanProcessor removes the given SpanProcessor from the list of SpanProcessors

akvanhar · 2019-10-21T18:09:17Z

sdk/trace/span_processor_test.go

@@ -48,10 +42,13 @@ func (t *testSpanProcesor) Shutdown() {

 func TestRegisterSpanProcessort(t *testing.T) {
 	name := "Register span processor before span starts"
+	tp, _ := sdktrace.NewProvider(
+		sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}))


This line is repeated a lot.
suggestion:
Set var testConfig = sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()} and use that in WithConfig

paivagustavo · 2019-10-21T19:26:15Z

api/trace/noop_trace_manager.go

@@ -0,0 +1,24 @@
+// Copyright 2019, OpenTelemetry Authors


Should this file be named noop_trace_provider.go?

rghetia · 2019-10-22T18:03:51Z

My concern about naming is that
   tracer := trace.GlobalProvider().GetTracer(name) 
is fairly long. Renaming GetTracer() to Get() may help. In #216 I've proposed that globals move out of the core packages, so then I could imagine a package named global that would give us calls like:
  tracer := global.GetTracer(name)
the Init() method proposed in OTEP 0005 would take both a metric and a trace provider, but the caller doesn't need to write "provider" when accessing the globals.

I'll take care of this in another PR.

jmacd · 2019-10-22T18:15:12Z

sdk/trace/benchmark_test.go

@@ -144,6 +145,7 @@ func BenchmarkSpanWithAttributes_all_2x(b *testing.B) {
 }

 func BenchmarkTraceID_DotString(b *testing.B) {
+	getTracer(b, "Benchmark traceID to string")


It just doesn't look like the tracer is used in this benchmark. No big deal.

rghetia requested review from bogdandrutu, jmacd, pjanotti, sjkaris and tedsuo as code owners October 21, 2019 13:56

jmacd approved these changes Oct 21, 2019

View reviewed changes

akvanhar reviewed Oct 21, 2019

View reviewed changes

paivagustavo reviewed Oct 21, 2019

View reviewed changes

rghetia added 15 commits October 21, 2019 13:55

named tracer prototype

d65643c

rename Manager to Provider.

5928b00

fix compile error after merge.

251c345

rename Tracer method to GetTracer

9da6f19

provider with options.

d46e945

update test.

df5b580

cleanup make circl-ci

b83da1c

remove global config.

a1e8b2b

some cleanup.

7c3a2ca

use provider for bridge

95eef96

update propagation test.

ccd0949

update examples and plugins.

73507d5

remove GlobalTracer methods.

78521de

fix review comments.

ee942b7

some more cleanup.

409bb97

rghetia force-pushed the named_tracer branch from c94f54c to 409bb97 Compare October 22, 2019 18:02

akvanhar approved these changes Oct 22, 2019

View reviewed changes

jmacd approved these changes Oct 22, 2019

View reviewed changes

remove unnecessary getTracer call in benchmark test.

ce9ca0e

rghetia merged commit b04db14 into open-telemetry:master Oct 22, 2019

This was referenced Oct 23, 2019

init go modules for stackdriver exporter #236

Merged

Can't re-call SetGlobalTracer with two different types of tracer #198

Closed

move global trace provider api to global package. #240

Merged

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Named tracer prototype #227

WIP: Named tracer prototype #227

rghetia commented Oct 21, 2019

jmacd left a comment

jmacd Oct 21, 2019

rghetia Oct 22, 2019

jmacd Oct 22, 2019

rghetia Oct 22, 2019

jmacd Oct 21, 2019

jmacd Oct 21, 2019

rghetia Oct 22, 2019

jmacd commented Oct 21, 2019

akvanhar Oct 21, 2019

rghetia Oct 22, 2019

akvanhar Oct 21, 2019

rghetia Oct 22, 2019

akvanhar Oct 21, 2019

rghetia Oct 22, 2019

akvanhar Oct 21, 2019

rghetia Oct 22, 2019

akvanhar Oct 21, 2019

rghetia Oct 22, 2019

paivagustavo Oct 21, 2019

rghetia Oct 22, 2019

rghetia commented Oct 22, 2019

jmacd Oct 22, 2019

WIP: Named tracer prototype #227

WIP: Named tracer prototype #227

Conversation

rghetia commented Oct 21, 2019

jmacd 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

jmacd commented Oct 21, 2019

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

rghetia commented Oct 22, 2019

Choose a reason for hiding this comment