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

Allow replacing trace SDK #1234

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

dashpole
Copy link
Collaborator

@dashpole dashpole commented Oct 14, 2020

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

@dashpole dashpole requested review from rakyll, rghetia and a team as code owners October 14, 2020 00:01
@google-cla google-cla bot added the cla: yes label Oct 14, 2020
trace/trace_api.go Outdated Show resolved Hide resolved
@nilebox
Copy link

nilebox commented Oct 14, 2020

@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 dashpole force-pushed the replaceable_sdk branch 2 times, most recently from c18a8c3 to 14793a8 Compare October 14, 2020 17:21
@dashpole
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@nilebox nilebox merged commit fc3822b into census-instrumentation:master Oct 22, 2020
@nilebox
Copy link

nilebox commented Oct 22, 2020

@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.

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

Successfully merging this pull request may close these issues.

2 participants