Deprecate NewSpanXXX in the favor of StartSpan #708

rakyll · 2018-04-16T20:46:36Z

Required to support the Go execution tracer spans.

Fixes #694.

odeke-em · 2018-04-16T20:49:06Z

exporter/stackdriver/trace.go

@@ -117,11 +117,14 @@ func (e *traceExporter) uploadSpans(spans []*trace.SpanData) {
 		req.Spans = append(req.Spans, protoFromSpanData(span, e.projectID))
 	}
 	// Create a never-sampled span to prevent traces associated with exporter.
-	span := trace.NewSpan("go.opencensus.io/exporter/stackdriver.uploadSpans", nil, trace.StartOptions{Sampler: trace.NeverSample()})
+	ctx, span := trace.StartSpan( // TODO: add timeouts


@rakyll "timeouts" for what?

It is a carry over from the old code. TODOs should have an owner. @Ramonza, please always add an owner for TODOS.

odeke-em · 2018-04-16T20:49:56Z

exporter/stackdriver/trace_test.go

 	for i := 0; i < 35; i++ {
-		_, span := trace.StartSpan(context.Background(), "span")
+		_, span := trace.StartSpan(context.Background(), "span", trace.WithSampler(trace.AlwaysSample()))


Why not leave it as it was before?

I don't see any value why we set global settings in the tests unless we are not testing the ability to have global settings.

odeke-em · 2018-04-16T20:51:36Z

trace/trace.go

-	span := NewSpan(name, parentSpan, StartOptions{})
+// If WithRemoteParent option is provided, started span will be a child
+// of the given remote parent.
+func StartSpan(ctx context.Context, name string, o ...StartOption) (context.Context, *Span) {


Hand clap, this is a great change! It takes away the previous non-ideal usage of:

trace.NewSpan

trace.WithSpan to get the context

odeke-em · 2018-04-16T20:52:05Z

trace/trace.go

+	}
+
+	var span *Span
+	if (opts.remoteParent != SpanContext{}) {


Umm, please remove the brackets.

Doesn't compile without the brackets.

I guess I shouldn't spend too much time on this but if you'd like

if opts.remoteParent != (SpanContext{}) {

is more conformant to the general style in this repo

odeke-em · 2018-04-16T20:53:29Z

trace/trace.go

+
+	var span *Span
+	if (opts.remoteParent != SpanContext{}) {
+		span = startSpanInternal(name, opts.remoteParent != SpanContext{}, opts.remoteParent, true, opts)


We've already evaluated this condition opts.remoteParent != SpanContext{} so perhaps let's improve it with

if hasRemoteParent := opts.remoteParent != SpanContext{}; hasRemoteParent { span = startSpanInternal(name, hasRemoteParent, opts.remoteParent, true, opts) } else { ... }

hasRemoteParent would always be true. Fixed this.

odeke-em · 2018-04-16T21:41:42Z

trace/trace.go

 		parentSpanContext = parent.SpanContext()
 	}
-	return startSpanInternal(name, hasParent, parentSpanContext, false, o)
+	return startSpanInternal(name, true, parentSpanContext, false, o)


Wait, why has this changed? If parent is nil, why should hasParent or remoteParent be true?

Oops, fixed.

odeke-em · 2018-04-16T21:56:15Z

trace/trace.go

+
+	var span *Span
+	if opts.remoteParent != (SpanContext{}) {
+		span = startSpanInternal(name, true, opts.remoteParent, true, opts)


Am not sure if this is the right change, not every span has a remote parent.

Rather, not every span has a parent

If opts.remoteParent is non-zero, the span has a parent.

Required to support the Go execution tracer spans. Fixes census-instrumentation#694.

rakyll · 2018-04-17T17:28:37Z

PTAL

bogdandrutu · 2018-04-17T20:20:53Z

trace/trace.go

+
+// WithSampler makes new spans to be be created with a custom sampler.
+// Otherwise, the global sampler is used.
+func WithSampler(sampler Sampler) StartOption {


I think this will confuse people, WithSpan vs WithSampler (or any other With) are completely different but they look so alike.

I'd suggest us to rename WithSpan to NewContext to avoid that confusion.

Given it is a such a critical API, I didn't want to break it. But did it.

I think this will be such a confusion for users. In all the languages we call this WithSpan, I would rather rename these WithSampler or simply use a builder pattern for the StartOptions.

Looking more into other libraries like https://godoc.org/google.golang.org/grpc I see that with is a pattern used for optional things. So I am fine with this approach.

bogdandrutu · 2018-04-17T20:22:29Z

trace/trace.go

+}
+
+// WithRemoteParent makes new spans to be the child of the given part.
+func WithRemoteParent(parent SpanContext) StartOption {


Need explanation of when to use it, what is the difference between this and getting parent from the ctxt vs this (when does it happen to get parent from the ctxt), etc.

rakyll

@bogdandrutu PTAL.

rakyll · 2018-04-17T21:28:51Z

trace/trace.go

+
+// WithSampler makes new spans to be be created with a custom sampler.
+// Otherwise, the global sampler is used.
+func WithSampler(sampler Sampler) StartOption {


I'd suggest us to rename WithSpan to NewContext to avoid that confusion.

Given it is a such a critical API, I didn't want to break it. But did it.

rakyll · 2018-04-17T22:30:41Z

trace/trace.go

+}
+
+// WithRemoteParent makes new spans to be the child of the given part.
+func WithRemoteParent(parent SpanContext) StartOption {


bogdandrutu

LGTM in general

bogdandrutu · 2018-04-17T23:41:09Z

trace/trace.go

+
+// WithSampler makes new spans to be be created with a custom sampler.
+// Otherwise, the global sampler is used.
+func WithSampler(sampler Sampler) StartOption {


Looking more into other libraries like https://godoc.org/google.golang.org/grpc I see that with is a pattern used for optional things. So I am fine with this approach.

bogdandrutu · 2018-04-17T23:42:48Z

trace/trace.go

+//
+// If there is already a parent in the incoming context.Context,
+// this option replaces it with the given parent.
+func WithRemoteParent(parent SpanContext) StartOption {


I think this is not actually an "optional" thing, in case of a grpc/http server plugins this is required to be set and in all the other cases this is required to not be set. I will have 2 apis StartSpan and StartSpanWithRemoteParent and make this a required field in the second API.

I see your point. I added StartSpanWithRemoteParent and removed the WithRemoteParent.

bogdandrutu

LGTM

rakyll · 2018-04-18T16:06:59Z

@odeke-em @Ramonza can you take a final look for Go readability before I merge?

semistrict

two minor comments, otherwise LGTM

semistrict · 2018-04-18T16:58:38Z

trace/trace.go

@@ -125,32 +132,69 @@ type StartOptions struct {
 	SpanKind int
 }

+// StartOption apply changes to StartOptions.
+type StartOption func(*StartOptions)


does StartOptions need to be public?

I guess it does for backwards compatibility but maybe we should mark it as deprecated

I think we should keep it as it is. It allows ochttp and ocgrpc to provide it as a configuration field.

semistrict · 2018-04-18T17:04:03Z

trace/trace.go

 // StartSpan starts a new child span of the current span in the context. If
 // there is no span in the context, creates a new trace and span.
+func StartSpan(ctx context.Context, name string, o ...StartOption) (context.Context, *Span) {
+	var opts StartOptions
+


rakyll requested review from semistrict, bogdandrutu and odeke-em April 16, 2018 20:46

chingor13 added the in progress label Apr 16, 2018

odeke-em reviewed Apr 16, 2018

View reviewed changes

rakyll force-pushed the startspan branch from 808d94c to 613a2da Compare April 16, 2018 21:11

rakyll mentioned this pull request Apr 16, 2018

Proposal: StartSpan with functional options #696

Closed

rakyll force-pushed the startspan branch from 613a2da to a5e3285 Compare April 16, 2018 21:33

odeke-em reviewed Apr 16, 2018

View reviewed changes

rakyll force-pushed the startspan branch from a5e3285 to a5fe0f2 Compare April 16, 2018 22:30

Deprecate NewSpanXXX in the favor of StartSpan

a8312a3

Required to support the Go execution tracer spans. Fixes census-instrumentation#694.

rakyll force-pushed the startspan branch from a5fe0f2 to a8312a3 Compare April 16, 2018 22:31

bogdandrutu reviewed Apr 17, 2018

View reviewed changes

Address feedback

a2bc4d6

rakyll commented Apr 17, 2018

View reviewed changes

bogdandrutu reviewed Apr 17, 2018

View reviewed changes

bogdandrutu approved these changes Apr 18, 2018

View reviewed changes

semistrict approved these changes Apr 18, 2018

View reviewed changes

Address feedback

aeae007

rakyll force-pushed the startspan branch from 75b8320 to aeae007 Compare April 18, 2018 17:35

rakyll merged commit 730e847 into census-instrumentation:master Apr 18, 2018

rakyll deleted the startspan branch April 18, 2018 17:46

chingor13 removed the in progress label Apr 18, 2018

Deprecate NewSpanXXX in the favor of StartSpan #708

Deprecate NewSpanXXX in the favor of StartSpan #708

Conversation

rakyll commented Apr 16, 2018

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

rakyll commented Apr 17, 2018

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

rakyll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu 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

bogdandrutu left a comment

Choose a reason for hiding this comment

rakyll commented Apr 18, 2018

semistrict 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