Repository: open-telemetry/opentelemetry-go
Pull request #1363 (2020-11-21 02:15:07)

Enable support for externally-defined ID generators
* Moved the SDK's `internal.IDGenerator` interface to the `sdk/trace`
package.
* Added `trace.WithIDGenerator()` `TracerProviderOption`.

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363?src=pr&el=h1) Report
> Merging [#1363](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363?src=pr&el=desc) (655ab60) into [master](https://codecov.io/gh/open-telemetry/opentelemetry-go/commit/e0819780f9d4ea1e8169109cd71b848fe08f788c?el=desc) (e081978) will **increase** coverage by `0.2%`.
> The diff coverage is `71.4%`.

[![Impacted file tree graph](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/graphs/tree.svg?width=650&height=150&src=pr&token=8efTmh4kvf)](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           master   #1363     +/-   ##
========================================
+ Coverage    77.4%   77.7%   +0.2%     
========================================
  Files         124     123      -1     
  Lines        6104    6082     -22     
========================================
  Hits         4730    4730             
+ Misses       1125    1103     -22     
  Partials      249     249             
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [sdk/trace/provider.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-c2RrL3RyYWNlL3Byb3ZpZGVyLmdv) | `92.3% <0.0%> (-2.1%)` | :arrow_down: |
| [sdk/trace/id\_generator.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-c2RrL3RyYWNlL2lkX2dlbmVyYXRvci5nbw==) | `100.0% <100.0%> (ø)` | |
| [metric/number/kind\_string.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-bWV0cmljL251bWJlci9raW5kX3N0cmluZy5nbw==) | `33.3% <0.0%> (+16.6%)` | :arrow_up: |
| [metric/instrumentkind\_string.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-bWV0cmljL2luc3RydW1lbnRraW5kX3N0cmluZy5nbw==) | `33.3% <0.0%> (+23.3%)` | :arrow_up: |
| [label/type\_string.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-bGFiZWwvdHlwZV9zdHJpbmcuZ28=) | `33.3% <0.0%> (+26.1%)` | :arrow_up: |
| [sdk/export/metric/exportkind\_string.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1363/diff?src=pr&el=tree#diff-c2RrL2V4cG9ydC9tZXRyaWMvZXhwb3J0a2luZF9zdHJpbmcuZ28=) | `100.0% <0.0%> (+42.8%)` | :arrow_up: |

jmacd: I made a note in the associated spec PR (https://github.com/open-telemetry/opentelemetry-specification/pull/1006#discussion_r520693556) that I'd probably rather see two methods, where one returns a single SpanID (taking a mutex once) and one returns a pair of TraceID and SpanID (taking a mutex once). What do you think?
krnowak: > I made a note in the associated spec PR ([open-telemetry/opentelemetry-specification#1006 (comment)](https://github.com/open-telemetry/opentelemetry-specification/pull/1006#discussion_r520693556)) that I'd probably rather see two methods, where one returns a single SpanID (taking a mutex once) and one returns a pair of TraceID and SpanID (taking a mutex once). What do you think?

Sounds useful as this seems to be how the ID generator is going to be used. Would there be a case where we only wanted to generate a trace ID?
MrAlias: > I made a note in the associated spec PR ([open-telemetry/opentelemetry-specification#1006 (comment)](https://github.com/open-telemetry/opentelemetry-specification/pull/1006#discussion_r520693556)) that I'd probably rather see two methods, where one returns a single SpanID (taking a mutex once) and one returns a pair of TraceID and SpanID (taking a mutex once). What do you think?

Yeah, I think this is indeed going to be how this will be used.

> Would there be a case where we only wanted to generate a trace ID?

I feel like that would be the corner case anyway so throwing away that returned value wouldn't be too much of an issue.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 058deb3f..1e2ea008 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,9 +8,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+
+- `trace.WithIDGenerator()` `TracerProviderOption`. (#1363)
+
 ### Changed
 
 - Move the OpenCensus example into `example` directory. (#1359)
+- Moved the SDK's `internal.IDGenerator` interface in to the `sdk/trace` package to enable support for externally-defined ID generators. (#1363)
 - `NewExporter` and `Start` functions in `go.opentelemetry.io/otel/exporters/otlp` now receive `context.Context` as a first parameter. (#1357)
 - Zipkin exporter relies on the status code for success rather than body read but still read the response body. (#1328)
 
diff --git a/sdk/trace/config.go b/sdk/trace/config.go
index 3fd37de2..42b1851f 100644
--- a/sdk/trace/config.go
+++ b/sdk/trace/config.go
@@ -16,7 +16,6 @@ package trace // import "go.opentelemetry.io/otel/sdk/trace"
 
 import (
 	"go.opentelemetry.io/otel/sdk/resource"
-	"go.opentelemetry.io/otel/sdk/trace/internal"
 )
 
 // Config represents the global tracing configuration.
@@ -25,7 +24,7 @@ type Config struct {
 	DefaultSampler Sampler
 
 	// IDGenerator is for internal use only.
-	IDGenerator internal.IDGenerator
+	IDGenerator IDGenerator
 
 	// MaxEventsPerSpan is max number of message events per span
 	MaxEventsPerSpan int
diff --git a/sdk/trace/id_generator.go b/sdk/trace/id_generator.go
index 3a4abe62..e60a421c 100644
--- a/sdk/trace/id_generator.go
+++ b/sdk/trace/id_generator.go
@@ -15,23 +15,30 @@
 package trace // import "go.opentelemetry.io/otel/sdk/trace"
 
 import (
+	"context"
+	crand "crypto/rand"
+	"encoding/binary"
 	"math/rand"
 	"sync"
 
 	"go.opentelemetry.io/otel/trace"
-
-	"go.opentelemetry.io/otel/sdk/trace/internal"
 )
 
-type defaultIDGenerator struct {
+// IDGenerator allows custom generators for TraceID and SpanID.
+type IDGenerator interface {
+	NewIDs(ctx context.Context) (trace.TraceID, trace.SpanID)
+	NewSpanID(ctx context.Context, traceID trace.TraceID) trace.SpanID
+}
+
+type randomIDGenerator struct {
 	sync.Mutex
 	randSource *rand.Rand
 }
 
-var _ internal.IDGenerator = &defaultIDGenerator{}
+var _ IDGenerator = &randomIDGenerator{}
 
 // NewSpanID returns a non-zero span ID from a randomly-chosen sequence.
-func (gen *defaultIDGenerator) NewSpanID() trace.SpanID {
+func (gen *randomIDGenerator) NewSpanID(ctx context.Context, traceID trace.TraceID) trace.SpanID {
 	gen.Lock()
 	defer gen.Unlock()
 	sid := trace.SpanID{}
@@ -39,12 +46,22 @@ func (gen *defaultIDGenerator) NewSpanID() trace.SpanID {
 	return sid
 }
 
-// NewTraceID returns a non-zero trace ID from a randomly-chosen sequence.
-// mu should be held while this function is called.
-func (gen *defaultIDGenerator) NewTraceID() trace.TraceID {
+// NewIDs returns a non-zero trace ID and a non-zero span ID from a
+// randomly-chosen sequence.
+func (gen *randomIDGenerator) NewIDs(ctx context.Context) (trace.TraceID, trace.SpanID) {
 	gen.Lock()
 	defer gen.Unlock()
 	tid := trace.TraceID{}
 	gen.randSource.Read(tid[:])
-	return tid
+	sid := trace.SpanID{}
+	gen.randSource.Read(sid[:])
+	return tid, sid
+}
+
+func defaultIDGenerator() IDGenerator {
+	gen := &randomIDGenerator{}
+	var rngSeed int64
+	_ = binary.Read(crand.Reader, binary.LittleEndian, &rngSeed)
+	gen.randSource = rand.New(rand.NewSource(rngSeed))
+	return gen
 }
diff --git a/sdk/trace/internal/internal.go b/sdk/trace/internal/internal.go
deleted file mode 100644
index 08898efa..00000000
--- a/sdk/trace/internal/internal.go
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright The OpenTelemetry Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-// Package internal provides trace internals.
-package internal
-
-import "go.opentelemetry.io/otel/trace"
-
-// IDGenerator allows custom generators for TraceId and SpanId.
-type IDGenerator interface {
-	NewTraceID() trace.TraceID
-	NewSpanID() trace.SpanID
-}
diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go
index 2ed731ac..4c6e6e2e 100644
--- a/sdk/trace/provider.go
+++ b/sdk/trace/provider.go
@@ -66,7 +66,7 @@ func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider {
 	}
 	tp.config.Store(&Config{
 		DefaultSampler:       ParentBased(AlwaysSample()),
-		IDGenerator:          defIDGenerator(),
+		IDGenerator:          defaultIDGenerator(),
 		MaxAttributesPerSpan: DefaultMaxAttributesPerSpan,
 		MaxEventsPerSpan:     DefaultMaxEventsPerSpan,
 		MaxLinksPerSpan:      DefaultMaxLinksPerSpan,
@@ -231,3 +231,10 @@ func WithResource(r *resource.Resource) TracerProviderOption {
 		opts.config.Resource = r
 	}
 }
+
+// WithIDGenerator option registers an IDGenerator with the TracerProvider.
+func WithIDGenerator(g IDGenerator) TracerProviderOption {
+	return func(opts *TracerProviderConfig) {
+		opts.config.IDGenerator = g
+	}
+}
diff --git a/sdk/trace/span.go b/sdk/trace/span.go
index 5ee2a8bb..fb3c6382 100644
--- a/sdk/trace/span.go
+++ b/sdk/trace/span.go
@@ -15,6 +15,7 @@
 package trace // import "go.opentelemetry.io/otel/sdk/trace"
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"reflect"
@@ -306,7 +307,7 @@ func (s *span) addChild() {
 	s.mu.Unlock()
 }
 
-func startSpanInternal(tr *tracer, name string, parent trace.SpanContext, remoteParent bool, o *trace.SpanConfig) *span {
+func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trace.SpanContext, remoteParent bool, o *trace.SpanConfig) *span {
 	var noParent bool
 	span := &span{}
 	span.spanContext = parent
@@ -314,10 +315,13 @@ func startSpanInternal(tr *tracer, name string, parent trace.SpanContext, remote
 	cfg := tr.provider.config.Load().(*Config)
 
 	if parent == emptySpanContext {
-		span.spanContext.TraceID = cfg.IDGenerator.NewTraceID()
+		// Generate both TraceID and SpanID
+		span.spanContext.TraceID, span.spanContext.SpanID = cfg.IDGenerator.NewIDs(ctx)
 		noParent = true
+	} else {
+		// TraceID already exists, just generate a SpanID
+		span.spanContext.SpanID = cfg.IDGenerator.NewSpanID(ctx, parent.TraceID)
 	}
-	span.spanContext.SpanID = cfg.IDGenerator.NewSpanID()
 	data := samplingData{
 		noParent:     noParent,
 		remoteParent: remoteParent,
diff --git a/sdk/trace/trace.go b/sdk/trace/trace.go
deleted file mode 100644
index 302ca13c..00000000
--- a/sdk/trace/trace.go
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright The OpenTelemetry Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package trace // import "go.opentelemetry.io/otel/sdk/trace"
-
-import (
-	crand "crypto/rand"
-	"encoding/binary"
-	"math/rand"
-
-	"go.opentelemetry.io/otel/sdk/trace/internal"
-)
-
-func defIDGenerator() internal.IDGenerator {
-	gen := &defaultIDGenerator{}
-	var rngSeed int64
-	_ = binary.Read(crand.Reader, binary.LittleEndian, &rngSeed)
-	gen.randSource = rand.New(rand.NewSource(rngSeed))
-	return gen
-}
diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go
index 872c37d8..ea90bb65 100644
--- a/sdk/trace/tracer.go
+++ b/sdk/trace/tracer.go
@@ -47,7 +47,7 @@ func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanO
 		}
 	}
 
-	span := startSpanInternal(tr, name, parentSpanContext, remoteParent, config)
+	span := startSpanInternal(ctx, tr, name, parentSpanContext, remoteParent, config)
 	for _, l := range links {
 		span.addLink(l)
 	}


Test patch:
diff --git a/sdk/trace/sampling_test.go b/sdk/trace/sampling_test.go
index 3c998773..6ae4afff 100644
--- a/sdk/trace/sampling_test.go
+++ b/sdk/trace/sampling_test.go
@@ -15,6 +15,7 @@
 package trace
 
 import (
+	"context"
 	"fmt"
 	"math/rand"
 	"testing"
@@ -168,7 +169,7 @@ func TestTraceIdRatioSamplesInclusively(t *testing.T) {
 		numSamplers = 1000
 		numTraces   = 100
 	)
-	idg := defIDGenerator()
+	idg := defaultIDGenerator()
 
 	for i := 0; i < numSamplers; i++ {
 		ratioLo, ratioHi := rand.Float64(), rand.Float64()
@@ -178,7 +179,7 @@ func TestTraceIdRatioSamplesInclusively(t *testing.T) {
 		samplerHi := TraceIDRatioBased(ratioHi)
 		samplerLo := TraceIDRatioBased(ratioLo)
 		for j := 0; j < numTraces; j++ {
-			traceID := idg.NewTraceID()
+			traceID, _ := idg.NewIDs(context.Background())
 
 			params := SamplingParameters{TraceID: traceID}
 			if samplerLo.ShouldSample(params).Decision == RecordAndSample {
diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
index da9e46ae..8ccd4404 100644
--- a/sdk/trace/trace_test.go
+++ b/sdk/trace/trace_test.go
@@ -213,7 +213,7 @@ func TestRecordingIsOn(t *testing.T) {
 }
 
 func TestSampling(t *testing.T) {
-	idg := defIDGenerator()
+	idg := defaultIDGenerator()
 	const total = 10000
 	for name, tc := range map[string]struct {
 		sampler       Sampler
@@ -263,9 +263,10 @@ func TestSampling(t *testing.T) {
 			for i := 0; i < total; i++ {
 				ctx := context.Background()
 				if tc.parent {
+					tid, sid := idg.NewIDs(ctx)
 					psc := trace.SpanContext{
-						TraceID: idg.NewTraceID(),
-						SpanID:  idg.NewSpanID(),
+						TraceID: tid,
+						SpanID:  sid,
 					}
 					if tc.sampledParent {
 						psc.TraceFlags = trace.FlagsSampled