Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Deprecate NewSpanXXX in the favor of StartSpan #708

Merged
merged 3 commits into from
Apr 18, 2018
Merged

Deprecate NewSpanXXX in the favor of StartSpan #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8312a3
Deprecate NewSpanXXX in the favor of StartSpan
rakyll Apr 16, 2018
a2bc4d6
Address feedback
rakyll Apr 17, 2018
aeae007
Address feedback
rakyll Apr 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions exporter/stackdriver/stackdriver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,13 @@ func TestExport(t *testing.T) {

trace.ApplyConfig(trace.Config{DefaultSampler: trace.AlwaysSample()})

span := trace.NewSpan("custom-span", nil, trace.StartOptions{})
_, span := trace.StartSpan(context.Background(), "custom-span")
time.Sleep(10 * time.Millisecond)
span.End()

// Test HTTP spans

handler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {

_, backgroundSpan := trace.StartSpan(context.Background(), "BackgroundWork")
spanContext := backgroundSpan.SpanContext()
time.Sleep(10 * time.Millisecond)
Expand Down
7 changes: 3 additions & 4 deletions exporter/stackdriver/stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,11 @@ func (e *statsExporter) Flush() {
}

func (e *statsExporter) uploadStats(vds []*view.Data) error {
span := trace.NewSpan(
ctx, span := trace.StartSpan(
context.Background(),
"go.opencensus.io/exporter/stackdriver.uploadStats",
nil,
trace.StartOptions{Sampler: trace.NeverSample()},
trace.WithSampler(trace.NeverSample()),
)
ctx := trace.WithSpan(context.Background(), span)
defer span.End()

for _, vd := range vds {
Expand Down
7 changes: 5 additions & 2 deletions exporter/stackdriver/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

@rakyll "timeouts" for what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

context.Background(),
"go.opencensus.io/exporter/stackdriver.uploadSpans",
trace.WithSampler(trace.NeverSample()),
)
defer span.End()
span.AddAttributes(trace.Int64Attribute("num_spans", int64(len(spans))))

ctx := trace.WithSpan(context.Background(), span) // TODO: add timeouts
err := e.client.BatchWriteSpans(ctx, &req)
if err != nil {
span.SetStatus(trace.Status{Code: 2, Message: err.Error()})
Expand Down
8 changes: 5 additions & 3 deletions exporter/stackdriver/trace_proto_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,21 @@ func (t *testExporter) ExportSpan(s *trace.SpanData) {
}

func TestExportTrace(t *testing.T) {
ctx := context.Background()

var te testExporter
trace.RegisterExporter(&te)
defer trace.UnregisterExporter(&te)

span0 := trace.NewSpanWithRemoteParent(
ctx, span0 := trace.StartSpanWithRemoteParent(
ctx,
"span0",
trace.SpanContext{
TraceID: traceID,
SpanID: spanID,
TraceOptions: 1,
},
trace.StartOptions{})
ctx := trace.WithSpan(context.Background(), span0)
)
{
ctx1, span1 := trace.StartSpan(ctx, "span1")
{
Expand Down
3 changes: 1 addition & 2 deletions exporter/stackdriver/trace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,8 @@ func TestBundling(t *testing.T) {
}
trace.RegisterExporter(exporter)

trace.ApplyConfig(trace.Config{DefaultSampler: trace.AlwaysSample()})
for i := 0; i < 35; i++ {
_, span := trace.StartSpan(context.Background(), "span")
_, span := trace.StartSpan(context.Background(), "span", trace.WithSampler(trace.AlwaysSample()))
Copy link
Member

Choose a reason for hiding this comment

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

Why not leave it as it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

span.End()
}

Expand Down
5 changes: 1 addition & 4 deletions plugin/ocgrpc/grpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,7 @@ func TestClientHandler(t *testing.T) {
t.Fatal(err)
}

span := trace.NewSpan("/foo", nil, trace.StartOptions{
Sampler: trace.AlwaysSample(),
})
ctx = trace.WithSpan(ctx, span)
ctx, _ = trace.StartSpan(ctx, "/foo", trace.WithSampler(trace.AlwaysSample()))

var handler ClientHandler
ctx = handler.TagRPC(ctx, &stats.RPCTagInfo{
Expand Down
26 changes: 12 additions & 14 deletions plugin/ocgrpc/trace_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,9 @@ const traceContextKey = "grpc-trace-bin"
func (c *ClientHandler) traceTagRPC(ctx context.Context, rti *stats.RPCTagInfo) context.Context {
name := strings.TrimPrefix(rti.FullMethodName, "/")
name = strings.Replace(name, "/", ".", -1)
span := trace.NewSpan(name, trace.FromContext(ctx), trace.StartOptions{
Sampler: c.StartOptions.Sampler,
SpanKind: trace.SpanKindClient,
}) // span is ended by traceHandleRPC
ctx = trace.WithSpan(ctx, span)
ctx, span := trace.StartSpan(ctx, name,
trace.WithSampler(c.StartOptions.Sampler),
trace.WithSpanKind(trace.SpanKindClient)) // span is ended by traceHandleRPC
traceContextBinary := propagation.Binary(span.SpanContext())
return metadata.AppendToOutgoingContext(ctx, traceContextKey, string(traceContextBinary))
}
Expand All @@ -52,11 +50,6 @@ func (c *ClientHandler) traceTagRPC(ctx context.Context, rti *stats.RPCTagInfo)
//
// It returns ctx, with the new trace span added.
func (s *ServerHandler) traceTagRPC(ctx context.Context, rti *stats.RPCTagInfo) context.Context {
opts := trace.StartOptions{
Sampler: s.StartOptions.Sampler,
SpanKind: trace.SpanKindServer,
}

md, _ := metadata.FromIncomingContext(ctx)
name := strings.TrimPrefix(rti.FullMethodName, "/")
name = strings.Replace(name, "/", ".", -1)
Expand All @@ -72,15 +65,20 @@ func (s *ServerHandler) traceTagRPC(ctx context.Context, rti *stats.RPCTagInfo)
traceContextBinary := []byte(traceContext[0])
parent, haveParent = propagation.FromBinary(traceContextBinary)
if haveParent && !s.IsPublicEndpoint {
span := trace.NewSpanWithRemoteParent(name, parent, opts)
return trace.WithSpan(ctx, span)
ctx, _ := trace.StartSpanWithRemoteParent(ctx, name, parent,
trace.WithSpanKind(trace.SpanKindServer),
trace.WithSampler(s.StartOptions.Sampler),
)
return ctx
}
}
span := trace.NewSpan(name, nil, opts)
ctx, span := trace.StartSpan(ctx, name,
trace.WithSpanKind(trace.SpanKindServer),
trace.WithSampler(s.StartOptions.Sampler))
if haveParent {
span.AddLink(trace.Link{TraceID: parent.TraceID, SpanID: parent.SpanID, Type: trace.LinkTypeChild})
}
return trace.WithSpan(ctx, span)
return ctx
}

func traceHandleRPC(ctx context.Context, rs stats.RPCStats) {
Expand Down
18 changes: 8 additions & 10 deletions plugin/ochttp/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,19 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

func (h *Handler) startTrace(w http.ResponseWriter, r *http.Request) (*http.Request, func()) {
opts := trace.StartOptions{
Sampler: h.StartOptions.Sampler,
SpanKind: trace.SpanKindServer,
}

name := spanNameFromURL(r.URL)
ctx := r.Context()
var span *trace.Span
sc, ok := h.extractSpanContext(r)
if ok && !h.IsPublicEndpoint {
span = trace.NewSpanWithRemoteParent(name, sc, opts)
ctx = trace.WithSpan(ctx, span)
ctx, span = trace.StartSpanWithRemoteParent(ctx, name, sc,
trace.WithSampler(h.StartOptions.Sampler),
trace.WithSpanKind(trace.SpanKindServer))
} else {
span = trace.NewSpan(name, nil, opts)
ctx, span = trace.StartSpan(ctx, name,
trace.WithSampler(h.StartOptions.Sampler),
trace.WithSpanKind(trace.SpanKindServer),
)
if ok {
span.AddLink(trace.Link{
TraceID: sc.TraceID,
Expand All @@ -99,9 +98,8 @@ func (h *Handler) startTrace(w http.ResponseWriter, r *http.Request) (*http.Requ
})
}
}
ctx = trace.WithSpan(ctx, span)
span.AddAttributes(requestAttrs(r)...)
return r.WithContext(trace.WithSpan(r.Context(), span)), span.End
return r.WithContext(ctx), span.End
}

func (h *Handler) extractSpanContext(r *http.Request) (trace.SpanContext, bool) {
Expand Down
7 changes: 4 additions & 3 deletions plugin/ochttp/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@ func (t *traceTransport) RoundTrip(req *http.Request) (*http.Response, error) {
name := spanNameFromURL(req.URL)
// TODO(jbd): Discuss whether we want to prefix
// outgoing requests with Sent.
parent := trace.FromContext(req.Context())
span := trace.NewSpan(name, parent, t.startOptions)
req = req.WithContext(trace.WithSpan(req.Context(), span))
_, span := trace.StartSpan(req.Context(), name,
trace.WithSampler(t.startOptions.Sampler),
trace.WithSpanKind(trace.SpanKindClient))

req = req.WithContext(trace.WithSpan(req.Context(), span))
if t.format != nil {
t.format.SpanContextToRequest(span.SpanContext(), req)
}
Expand Down
14 changes: 6 additions & 8 deletions plugin/ochttp/trace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ func (t testPropagator) SpanContextToRequest(sc trace.SpanContext, req *http.Req
}

func TestTransport_RoundTrip(t *testing.T) {
parent := trace.NewSpan("parent", nil, trace.StartOptions{})
ctx := context.Background()
ctx, parent := trace.StartSpan(ctx, "parent")
tests := []struct {
name string
parent *trace.Span
Expand Down Expand Up @@ -221,12 +222,9 @@ func TestEndToEnd(t *testing.T) {
url := serveHTTP(tt.handler, serverDone, serverReturn)

// Start a root Span in the client.
root := trace.NewSpan(
"top-level",
nil,
trace.StartOptions{})
ctx := trace.WithSpan(context.Background(), root)

ctx, root := trace.StartSpan(
context.Background(),
"top-level")
// Make the request.
req, err := http.NewRequest(
http.MethodPost,
Expand Down Expand Up @@ -278,7 +276,7 @@ func TestEndToEnd(t *testing.T) {
t.Errorf("Span name: %q; want %q", got, want)
}
default:
t.Fatalf("server or client span missing")
t.Fatalf("server or client span missing; kind = %v", sp.SpanKind)
}
}

Expand Down
63 changes: 53 additions & 10 deletions trace/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,14 @@ func FromContext(ctx context.Context) *Span {
}

// WithSpan returns a new context with the given Span attached.
//
// Deprecated: Use NewContext.
func WithSpan(parent context.Context, s *Span) context.Context {
return NewContext(parent, s)
}

// NewContext returns a new context with the given Span attached.
func NewContext(parent context.Context, s *Span) context.Context {
return context.WithValue(parent, contextKey{}, s)
}

Expand Down Expand Up @@ -125,32 +132,68 @@ type StartOptions struct {
SpanKind int
}

// StartOption apply changes to StartOptions.
type StartOption func(*StartOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

does StartOptions need to be public?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// WithSpanKind makes new spans to be created with the given kind.
func WithSpanKind(spanKind int) StartOption {
return func(o *StartOptions) {
o.SpanKind = spanKind
}
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return func(o *StartOptions) {
o.Sampler = sampler
}
}

// 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
var parent SpanContext
if p := FromContext(ctx); p != nil {
parent = p.spanContext
}
for _, op := range o {
op(&opts)
}
span := startSpanInternal(name, parent != SpanContext{}, parent, false, opts)
return NewContext(ctx, span), span
}

// StartSpanWithRemoteParent starts a new child span of the span from the given parent.
//
// This is provided as a convenience for WithSpan(ctx, NewSpan(...)). Use it
// if you require custom spans in addition to the default spans provided by
// ocgrpc, ochttp or similar framework integration.
func StartSpan(ctx context.Context, name string) (context.Context, *Span) {
parentSpan, _ := ctx.Value(contextKey{}).(*Span)
span := NewSpan(name, parentSpan, StartOptions{})
return WithSpan(ctx, span), span
// If the incoming context contains a parent, it ignores. StartSpanWithRemoteParent is
// preferred for cases where the parent is propagated via an incoming request.
func StartSpanWithRemoteParent(ctx context.Context, name string, parent SpanContext, o ...StartOption) (context.Context, *Span) {
var opts StartOptions
for _, op := range o {
op(&opts)
}
span := startSpanInternal(name, parent != SpanContext{}, parent, true, opts)
return NewContext(ctx, span), span
}

// NewSpan returns a new span.
//
// If parent is not nil, created span will be a child of the parent.
//
// Deprecated: Use StartSpan.
func NewSpan(name string, parent *Span, o StartOptions) *Span {
hasParent := false
var parentSpanContext SpanContext
if parent != nil {
hasParent = true
parentSpanContext = parent.SpanContext()
}
return startSpanInternal(name, hasParent, parentSpanContext, false, o)
return startSpanInternal(name, parent != nil, parentSpanContext, false, o)
}

// NewSpanWithRemoteParent returns a new span with the given parent SpanContext.
//
// Deprecated: Use StartSpanWithRemoteParent.
func NewSpanWithRemoteParent(name string, parent SpanContext, o StartOptions) *Span {
return startSpanInternal(name, true, parent, true, o)
}
Expand Down
Loading