Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggested helper function trace.Start(ctx context.Context, name string, opts ...StartOption) (context.Context, Span) #1090

Closed
pci opened this issue Aug 25, 2020 · 5 comments
Labels
area:trace Part of OpenTelemetry tracing enhancement New feature or request pkg:API Related to an API package
Milestone

Comments

@pci
Copy link

pci commented Aug 25, 2020

Suggestion:

The suggestion is to add a helper function trace.Start to api/trace to aid the happy path of creating a child span within a function. The function would look something like:

func Start(ctx context.Context, name string, opts ...StartOption) (context.Context, Span)
  parentSpan := trace.SpanFromContext(ctx)
  tracer := parentSpan.Tracer()
  return tracer.Start(ctx, name, opts...)
}

Reasoning:

In converting some go apps that use opencensus to opentelemetry, one thing that I've found to be a sticking point in the transition is the happy path where a function takes a context and wants to add a span underneath, e.g. in opencensus you can do:

import (
  "go.opencensus.io/trace"
)

func foo(ctx context.Context) error {
  ctx, span := trace.StartSpan(ctx, "child")
  defer span.End()

  // ...
}

whilst in opentelemetry you either have to repeat tracer := global.TraceProvider().Tracer("example.com/trace") in every package/function with a shared state of the tracer name, or since the context contains the span, and the span links to the tracer I found myself doing this a lot:

import (
  "go.opentelemetry.io/otel/api/trace"
)

func foo(ctx context.Context) error {
  parentSpan := trace.SpanFromContext(ctx)
  tracer := parentSpan.Tracer()
  ctx, span := tracer.Start(ctx, "child")
  defer span.End()

  // ...
}

In coming to opentelemetry I found this "one step beyond hello-world example" a stumbling block and a tad repetitive. I would suggest a helper function like the above would aid readability, and ease first-time user experience.

It also means that modules using this pattern can be imported by two distinct applications and it will extract their individually named tracers without having to share the tracer name across the two applications and library (e.g. tracer := global.TraceProvider().Tracer("shared-trace-name")). If the importing or calling code doesn't use opentelemetry, and the context contains no span then the span extraction falls back to NoopSpan and causes no runtime issues, so functions can be coded to be "opentelemetry-ready" even if the caller hasn't hooked in an exporter yet.

I would love to know others' opinions on if this would be a useful usability feature, and I'm happy to code up a PR if people think this is a good idea.

@Aneurysm9 Aneurysm9 added area:trace Part of OpenTelemetry tracing enhancement New feature or request pkg:API Related to an API package labels Aug 25, 2020
@seh
Copy link
Contributor

seh commented Oct 29, 2020

See #609 for precedent.

@jsvensson
Copy link

jsvensson commented Dec 11, 2020

In coming to opentelemetry I found this "one step beyond hello-world example" a stumbling block and a tad repetitive. I would suggest a helper function like the above would aid readability, and ease first-time user experience.

I'm coming from OpenTracing and making a test project using otel, and yeah, had this exact stumbling block. At first glance I thought I was supposed to keep passing the parent context without making a new child span for each level.

@jmacd
Copy link
Contributor

jmacd commented Dec 11, 2020

I wonder why the proposed signature isn't the only signature. I'm used to this idiom:

ctx, span := trace.Start(ctx, "op_name")
defer span.End()
// ...

pci pushed a commit to pci/opentelemetry-go that referenced this issue Mar 8, 2021
pci pushed a commit to pci/opentelemetry-go that referenced this issue Mar 8, 2021
pci pushed a commit to pci/opentelemetry-go that referenced this issue Mar 8, 2021
@pci
Copy link
Author

pci commented Mar 8, 2021

@seh @jsvensson @jmacd Sorry for spamming you all, but since you all showed an interested in this issue I thought you might be interested that I got around to coding up a PR #1674 for the proposal, if you want to follow along there.

@pci
Copy link
Author

pci commented Mar 26, 2021

Hello people from the future reading this probably via google!

Since this issue was opened best practise has formed and solidified. The full details can be found here but the short version is each of your packages should define their own Open Telemetry Tracer with your code entry point defining a TraceProvider, and the exporter will still receive a coherent trace. So there's no need for a shared trace name or to use the underlying tracer from the span received.

@pci pci closed this as completed Mar 26, 2021
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing enhancement New feature or request pkg:API Related to an API package
Projects
None yet
Development

No branches or pull requests

7 participants