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

new trace.Tracer interface is impractical #4714

Closed
Malufasu opened this issue Nov 13, 2023 · 9 comments
Closed

new trace.Tracer interface is impractical #4714

Malufasu opened this issue Nov 13, 2023 · 9 comments

Comments

@Malufasu
Copy link

Malufasu commented Nov 13, 2023

Problem Statement

The new trace.Tracer interface (v1.20.0 exposes a private func: tracer().
This stops users from wrapping it easily, as in this use case that does not implement the interface anymore:

type wrapedTracer struct{
   trace.Tracer
}

func (wp *wrappedTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span){
  spanCtx,span := wp.tracer.Start(ctx,spanName,opts...)
  // log the span ID or anything else
}

Proposed Solution

Remove the tracer() func from the exposed interface (if need be, use another interface internally for the lib)

@Malufasu Malufasu added the enhancement New feature or request label Nov 13, 2023
@pellared
Copy link
Member

pellared commented Nov 14, 2023

This interface does not conforms the regular Go API versioning policy as it was noted for a long time here: https://github.com/open-telemetry/opentelemetry-go/blob/v1.19.0/trace/trace.go#L489.

We have a need to add new functionality to the trace API interfaces therefore we decided to go with the "embeded" design (that is also used in metrics API) to break the compilation only once instead of doing it each time we want to add a new feature. We totally understand the inconvenience but we have not found any other way to add new functionality exposed by OTel APIs.

Related issues:

@pellared
Copy link
Member

pellared commented Nov 14, 2023

FYI @EdSchouten

@MrAlias
Copy link
Contributor

MrAlias commented Nov 14, 2023

This stops users from wrapping it easily, as in this use case that does not implement the interface anymore

Can you expand on this? That looks like code that will compile and function. Why does that not implement the interface anymore?

@dashpole
Copy link
Contributor

@Malufasu I think your code should work if you are using the API:

I modified the fib example to wrap the Tracer from otel: main...dashpole:opentelemetry-go:example_demonstration_wrapper

It compiles and runs without any issues.

@Malufasu
Copy link
Author

Malufasu commented Nov 15, 2023

@dashpole @MrAlias the fib example indeed works but I do something a bit different:

type wrappedTracer struct{
   trace.Tracer
}

func (wp *wrappedTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span){
  spanCtx,span := wp.tracer.Start(ctx,spanName,opts...)
  // log the span ID or anything else
}

func newWrappedTracer(name string) *wrappedTracer {
	return &wrappedTracer{Tracer: otel.Tracer(name)}
}

func someHandlingFunc (ctx context.Context, tracer trace.Tracer) error {
  _, span := tracer.Start(ctx, "Handling")
  // ...
  return span.End()
}

type duplicateTracer interface{
  Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span)
}

func someFixedHandlingFunc (ctx context.Context, tracer duplicateTracer) error {
  _, span := tracer.Start(ctx, "Handling")
  // ...
  return span.End()
}

func main() {
  tracer := newWrappedTracer("myProgram")
  runCtx, runSpan := tracer.Start(context.Background(), "Run") // This works
  someHandlingFunc(runCtx, tracer)                             // This does not since *wrappedTracer does not implement the private func
  someFixedHandlingFunc(runCtx, tracer)                        // This works again with a workaround
  runSpan.End()
}

Since my use case does not need to modify the trace.Tracer interface, I had first settled on using it in my internal functions signature, which cannot be done anymore.

This is a bit specific of a use case since wrapping functions this way is not the preferred way of doing things in golang, though (as far as I am aware). But it allowed for a program using the otel lib to replace it with the wrappedTracer painlessly.

@Malufasu
Copy link
Author

just to clarify my understanding: is calling otel.Tracer(name) at every Start() what you mean by "using the API" ?

@Malufasu I think your code should work if you are using the API:

I modified the fib example to wrap the Tracer from otel: main...dashpole:opentelemetry-go:example_demonstration_wrapper

It compiles and runs without any issues.

@dashpole
Copy link
Contributor

I just meant that tracer.Tracer is from the trace API package (go.opentelemetry.io/otel/trace.Tracer), since you didn't list the import. Sorry for the confusion.

See https://pkg.go.dev/go.opentelemetry.io/otel/trace#hdr-API_Implementations for the guidance around this. We would recommend:

type duplicateTracer interface{
  noop.Tracer
  Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span)
}

@Malufasu
Copy link
Author

Noted,
thank you for your answers, this implementation indeed fixes the issue :)

// This type will satisfy trace.Tracer interface
// and allow overriding what we want
type wrappedTracer struct{
   // embedded tracer (default behaviour)
   noop.Tracer
   // the tracer that will get wrapped
   wrapped trace.Tracer
}

func (wp *wrappedTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span){
  spanCtx,span := wp.wrapped.Start(ctx,spanName,opts...)
  // log the span ID or anything else
}

@Malufasu
Copy link
Author

Should I close this issue ?

@pellared pellared removed the enhancement New feature or request label Nov 15, 2023
@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants