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

Make all span starting APIs take a context #694

Closed
rakyll opened this issue Apr 10, 2018 · 7 comments
Closed

Make all span starting APIs take a context #694

rakyll opened this issue Apr 10, 2018 · 7 comments
Assignees

Comments

@rakyll
Copy link
Contributor

rakyll commented Apr 10, 2018

Go runtime/trace package is soon going to support user events. See http://tip.golang.org/pkg/runtime/trace.

In order to provide execution tracer tasks and spans from the OpenCensus library, we should make sure we are not dropping the parent runtime/trace tasks and spans.

Hence I am suggesting us to remove NewSpanXXX functions and replace them with:

func StartSpanWithOptions(ctx context.Contex, name string, o StartOptions) (context.Context, *Span)
func StartSpanWithRemoteParent(ctx context.Contex, name string, parent SpanContext o StartOptions) (context.Context, *Span)
@rakyll
Copy link
Contributor Author

rakyll commented Apr 10, 2018

/cc @bogdandrutu @hyangah

@hyangah
Copy link
Contributor

hyangah commented Apr 11, 2018

Alternatively, if the Span carries the associated context in it, you can do the following without changing the API.

type Span struct {
   ...
   tracedCtx context.Context
   traceEnd func()
   ...
}

func NewSpan(name string, parent *Span, o StartOptions) *Span {
    var parentSpanContext SpanContext
    pctx := context.Background()
    if parent != nil {
        parentSpanContext = parent.SpanContext()
        pctx = parent.tracedCtx
    }  
    return startSpanInternal(pctx, name, parent != nil, parentSpanContext, false, o)
  }

One caveat is the newly added reference to the context.Context object - the parent context and its ancestors will live until the Span is ready for being GC'd.

@semistrict
Copy link
Contributor

Proposal for this: #696
@rakyll what do you think?

@rakyll
Copy link
Contributor Author

rakyll commented Apr 12, 2018

@hyangah, we didn't want to pin the context given it is not recommended due to the different lifetimes of span and context objects. I think #696 is quite better.

@hyangah
Copy link
Contributor

hyangah commented Apr 12, 2018

If it's okay to deprecate old apis, great.

BTW the difference between span and context objects (so how the current API looks) will still prevent from wiring up the span's methods (e.g. Annotate) with trace.Log.

@semistrict
Copy link
Contributor

@rakyll just to confirm, the ultimate goal here: is the idea that we want to automatically invoke runtime/trace methods when the similar OpenCensus functions/methods are called? E.g. we would call "runtime/trace".NewContext from within trace.NewSpan, and "runtime/trace".Logf from within span.Annotatef?

rakyll added a commit to rakyll/opencensus-go that referenced this issue Apr 16, 2018
Required to support the Go execution tracer spans.

Fixes census-instrumentation#694.
rakyll added a commit to rakyll/opencensus-go that referenced this issue Apr 16, 2018
Required to support the Go execution tracer spans.

Fixes census-instrumentation#694.
rakyll added a commit to rakyll/opencensus-go that referenced this issue Apr 16, 2018
Required to support the Go execution tracer spans.

Fixes census-instrumentation#694.
rakyll added a commit to rakyll/opencensus-go that referenced this issue Apr 16, 2018
Required to support the Go execution tracer spans.

Fixes census-instrumentation#694.
rakyll added a commit to rakyll/opencensus-go that referenced this issue Apr 16, 2018
Required to support the Go execution tracer spans.

Fixes census-instrumentation#694.
@rakyll
Copy link
Contributor Author

rakyll commented Apr 17, 2018

@Ramonza

the ultimate goal here: is the idea that we want to automatically invoke runtime/trace methods when the similar OpenCensus functions/methods are called? E.g. we would call "runtime/trace".NewContext from within trace.NewSpan, and "runtime/trace".Logf from within span.Annotatef?

Yes, we are considering about creating a runtime/trace task for each OpenCensus span. So if the user wants to see the runtime events, they can momentarily turn on the execution tracer and output some data that correlates with their distributed span.

Maybe we should pin the ctx in order to support runtime/trace.Logf in the future but we always need the incoming context anyways in order to runtime/trace.

rakyll added a commit that referenced this issue Apr 18, 2018
Required to support the Go execution tracer spans.

Fixes #694.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants