census-instrumentation / opencensus-go

A stats collection and distributed tracing framework
http://opencensus.io
Apache License 2.0
2.06k stars 327 forks source link

Allow replacing trace SDK #1234

Closed dashpole closed 4 years ago

dashpole commented 4 years ago

What this PR does:

  1. Change Span from a struct to an interface.
  2. Add a Tracer interface
  3. Add an implementation of the Tracer interface using the existing Tracer functions (StartSpan, StartSpanWithRemoteParent, FromContext, and WithContext)
  4. Add a global DefaultTracer variable that can be set by users, and defaults to the existing Tracer implementation.
  5. Add accessors for Key and Value for Attributes

Motivation for the PR:

We would like to be able to assist users in their migration to OpenTelemetry. One way to do this is by writing an implementation of OpenCensus APIs which uses OpenTelemetry under the hood. This way, OpenCensus spans with an OpenTelemetry parent (or vice-versa) would be linked, and all telemetry would be exported through the same exporters (OpenTelemetry exporters in this case).

Rationale for the PR:

This adopts a similar model to OpenTelemetry, in which the SDK can be swapped out for a different implementation.

User Impact:

For users using the "typical" workflow, there should be no changes. For example,

ctx, span := trace.StartSpan(ctx, "OpenCensusSpan")
span.End()

still functions the same way. Even though the Span struct was changed to an interface, the same methods still exist. Only users that pass Spans would need to change when updating:

// Before this change
func main() {
    ctx, span := trace.StartSpan(ctx, "OpenCensusSpan")
    endSpan(span)
}
func endSpan(s *Span) {
    s.End()
}
// After this change
func main() {
    ctx, span := trace.StartSpan(ctx, "OpenCensusSpan")
    endSpan(span)
}
func endSpan(s Span) {
    s.End()
}

notice the *Span was changed to Span

This change also impacts the evaluation of Span against nil. For example,

// Before this change
span := trace.FromContext(ctx)
if span != nil {
    // do something
}

The above will no longer function as expected. This is because nil, when assigned to an interface, makes the interface non-nil: https://play.golang.org/p/BWutROXBt5y. Users should change to the following:

// After this change
span := trace.FromContext(ctx)
if span.IsRecordingEvents() {
  // do something
}

Example Usage:

See https://github.com/dashpole/opencensus-migration-go/tree/replace_oc_sdk#opencensus-migration-go for an overview, and https://github.com/dashpole/opencensus-migration-go/tree/replace_oc_sdk/migration for an implementation of the new Tracer and Span APIs.

@nilebox @jsuereth @james-bebbington

nilebox commented 4 years ago

@dashpole there is some nil pointer dereference panic in the CI tests.

go.opencensus.io/plugin/ochttp.(*trackingResponseWriter).end.func1()
    /home/travis/gopath/src/go.opencensus.io/plugin/ochttp/server.go:190 +0x134
dashpole commented 4 years ago

Tests were failing because the evaluation of nil changed on the interface. I had to make some updates. I added a section to the description on the change in behavior for evaluation against nil.

dashpole commented 4 years ago

Is this good to merge? I don't have any power here

nilebox commented 4 years ago

@dashpole merged! I'd recommend trying with a pinned commit dependency first if that works well for the OpenTelemetry shim first before tagging a release.