Decouple API from SDK (#977)

* Remove SDK from othttp instrumentation * Remove dependency on SDK in api/kv pkg Benchmark the kv package not the SDK here. * Update api/global benchmarks Move SDK related tests to SDK where applicable * Add internal testing SDK implementation To be used by the API for testing so it does not depend on the actual SDK. * Update api/global/internal to use internal/sdk * Fix lint on sdk/metric benchmark * Lint internal/sdk * Merge internal/sdk into api/trace/testtrace * Update Changelog
open-telemetry · Jul 28, 2020 · 2833212 · 2833212
1 parent 7f1dc4a
commit 2833212
Show file tree

Hide file tree

Showing 15 changed files with 710 additions and 451 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Zipkin exporter helpers: pipeline methods introduced, new exporter method adjusted. (#944)
 - The trace (`go.opentelemetry.io/otel/exporters/trace/stdout`) and metric (`go.opentelemetry.io/otel/exporters/metric/stdout`) `stdout` exporters are now merged into a single exporter at `go.opentelemetry.io/otel/exporters/stdout`. (#956)
 
+### Fixed
+
+- Remove default SDK dependencies from the `go.opentelemetry.io/otel/api` package. (#977)
+
 ## [0.9.0] - 2020-07-20
 
 ### Added

diff --git a/api/global/internal/benchmark_test.go b/api/global/internal/benchmark_test.go
@@ -21,48 +21,12 @@ import (
 	"go.opentelemetry.io/otel/api/global"
 	"go.opentelemetry.io/otel/api/global/internal"
 	"go.opentelemetry.io/otel/api/kv"
-	"go.opentelemetry.io/otel/api/metric"
-	"go.opentelemetry.io/otel/api/trace"
-	export "go.opentelemetry.io/otel/sdk/export/metric"
-	sdk "go.opentelemetry.io/otel/sdk/metric"
-	"go.opentelemetry.io/otel/sdk/metric/processor/test"
-	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
-var Must = metric.Must
-
-// benchFixture is copied from sdk/metric/benchmark_test.go.
-// TODO refactor to share this code.
-type benchFixture struct {
-	export.AggregatorSelector
-	accumulator *sdk.Accumulator
-	meter       metric.Meter
-	B           *testing.B
-}
-
-var _ metric.Provider = &benchFixture{}
-
-func newFixture(b *testing.B) *benchFixture {
-	b.ReportAllocs()
-	bf := &benchFixture{
-		B:                  b,
-		AggregatorSelector: test.AggregatorSelector(),
-	}
-
-	bf.accumulator = sdk.NewAccumulator(bf)
-	bf.meter = metric.WrapMeterImpl(bf.accumulator, "test")
-	return bf
-}
-
-func (*benchFixture) Process(export.Accumulation) error {
-	return nil
-}
-
-func (fix *benchFixture) Meter(_ string, _ ...metric.MeterOption) metric.Meter {
-	return fix.meter
-}
-
 func BenchmarkGlobalInt64CounterAddNoSDK(b *testing.B) {
+	// Compare with BenchmarkGlobalInt64CounterAddWithSDK() in
+	// ../../../sdk/metric/benchmark_test.go to see the overhead of the
+	// global no-op system against a registered SDK.
 	internal.ResetForTest()
 	ctx := context.Background()
 	sdk := global.Meter("test")
@@ -76,60 +40,15 @@ func BenchmarkGlobalInt64CounterAddNoSDK(b *testing.B) {
 	}
 }
 
-func BenchmarkGlobalInt64CounterAddWithSDK(b *testing.B) {
-	// Comapare with BenchmarkInt64CounterAdd() in ../../sdk/meter/benchmark_test.go
+func BenchmarkStartEndSpanNoSDK(b *testing.B) {
+	// Compare with BenchmarkStartEndSpan() in
+	// ../../../sdk/trace/benchmark_test.go.
+	internal.ResetForTest()
+	t := global.Tracer("Benchmark StartEndSpan")
 	ctx := context.Background()
-	fix := newFixture(b)
-
-	sdk := global.Meter("test")
-
-	global.SetMeterProvider(fix)
-
-	labs := []kv.KeyValue{kv.String("A", "B")}
-	cnt := Must(sdk).NewInt64Counter("int64.counter")
-
 	b.ResetTimer()
-
 	for i := 0; i < b.N; i++ {
-		cnt.Add(ctx, 1, labs...)
-	}
-}
-
-func BenchmarkStartEndSpan(b *testing.B) {
-	// Comapare with BenchmarkStartEndSpan() in ../../sdk/trace/benchmark_test.go
-	traceBenchmark(b, func(b *testing.B) {
-		t := global.Tracer("Benchmark StartEndSpan")
-		ctx := context.Background()
-		b.ResetTimer()
-		for i := 0; i < b.N; i++ {
-			_, span := t.Start(ctx, "/foo")
-			span.End()
-		}
-	})
-}
-
-func traceBenchmark(b *testing.B, fn func(*testing.B)) {
-	internal.ResetForTest()
-	b.Run("No SDK", func(b *testing.B) {
-		b.ReportAllocs()
-		fn(b)
-	})
-	b.Run("Default SDK (AlwaysSample)", func(b *testing.B) {
-		b.ReportAllocs()
-		global.SetTraceProvider(traceProvider(b, sdktrace.AlwaysSample()))
-		fn(b)
-	})
-	b.Run("Default SDK (NeverSample)", func(b *testing.B) {
-		b.ReportAllocs()
-		global.SetTraceProvider(traceProvider(b, sdktrace.NeverSample()))
-		fn(b)
-	})
-}
-
-func traceProvider(b *testing.B, sampler sdktrace.Sampler) trace.Provider {
-	tp, err := sdktrace.NewProvider(sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sampler}))
-	if err != nil {
-		b.Fatalf("Failed to create trace provider with sampler: %v", err)
+		_, span := t.Start(ctx, "/foo")
+		span.End()
 	}
-	return tp
 }
diff --git a/api/global/internal/meter_test.go b/api/global/internal/meter_test.go
@@ -28,6 +28,8 @@ import (
 	metrictest "go.opentelemetry.io/otel/internal/metric"
 )
 
+var Must = metric.Must
+
 // Note: Maybe this should be factored into ../../../internal/metric?
 type measured struct {
 	Name                   string

diff --git a/api/global/internal/trace_test.go b/api/global/internal/trace_test.go
@@ -18,32 +18,14 @@ import (
 	"context"
 	"testing"
 
-	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/assert"
 
 	"go.opentelemetry.io/otel/api/global"
 	"go.opentelemetry.io/otel/api/global/internal"
-	export "go.opentelemetry.io/otel/sdk/export/trace"
-	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/api/trace/testtrace"
 )
 
-type testSpanProcesor struct {
-	// Names of Spans started.
-	spansStarted []string
-	// Names of Spans ended.
-	spansEnded []string
-}
-
-func (t *testSpanProcesor) OnStart(s *export.SpanData) {
-	t.spansStarted = append(t.spansStarted, s.Name)
-}
-
-func (t *testSpanProcesor) OnEnd(s *export.SpanData) {
-	t.spansEnded = append(t.spansEnded, s.Name)
-}
-
-func (t *testSpanProcesor) Shutdown() {}
-
-func TestTraceDefaultSDK(t *testing.T) {
+func TestTraceWithSDK(t *testing.T) {
 	internal.ResetForTest()
 
 	ctx := context.Background()
@@ -56,13 +38,8 @@ func TestTraceDefaultSDK(t *testing.T) {
 		t.Errorf("failed to wrap function with span prior to initialization: %v", err)
 	}
 
-	tp, err := sdktrace.NewProvider(sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}))
-	if err != nil {
-		t.Fatal(err)
-	}
-	tsp := &testSpanProcesor{}
-	tp.RegisterSpanProcessor(tsp)
-
+	sr := new(testtrace.StandardSpanRecorder)
+	tp := testtrace.NewProvider(testtrace.WithSpanRecorder(sr))
 	global.SetTraceProvider(tp)
 
 	// This span was started before initialization, it is expected to be dropped.
@@ -83,7 +60,14 @@ func TestTraceDefaultSDK(t *testing.T) {
 		t.Errorf("failed to wrap function with span post initialization with new tracer: %v", err)
 	}
 
+	filterNames := func(spans []*testtrace.Span) []string {
+		names := make([]string, len(spans))
+		for i := range spans {
+			names[i] = spans[i].Name()
+		}
+		return names
+	}
 	expected := []string{"span2", "withSpan2", "span3", "withSpan3"}
-	require.Equal(t, tsp.spansStarted, expected)
-	require.Equal(t, tsp.spansEnded, expected)
+	assert.ElementsMatch(t, expected, filterNames(sr.Started()))
+	assert.ElementsMatch(t, expected, filterNames(sr.Completed()))
 }