Add RO/RW span interfaces (#1360)

* Store span data directly in the span

- Nesting only some of a span's data in a `data` field (with the rest
  of the data living direclty in the `span` struct) is confusing.
- export.SpanData is meant to be an immutable *snapshot* of a span,
  not the "authoritative" state of the span.
- Refactor attributesMap.toSpanData into toKeyValue and make it
  return a []label.KeyValue which is clearer than modifying a struct
  passed to the function.
- Read droppedCount from the attributesMap as a separate operation
  instead of setting it from within attributesMap.toSpanData.
- Set a span's end time in the span itself rather than in the
  SpanData to allow reading the span's end time after a span has
  ended.
- Set a span's end time as soon as possible within span.End so that
  we don't influence the span's end time with operations such as
  fetching span processors and generating span data.
- Remove error handling for uninitialized spans. This check seems to
  be necessary only because we used to have an *export.SpanData field
  which could be nil. Now that we no longer have this field I think we
  can safely remove the check. The error isn't used anywhere else so
  remove it, too.

* Store parent as trace.SpanContext

The spec requires that the parent field of a Span be a Span, a
SpanContext or null.

Rather than extracting the parent's span ID from the trace.SpanContext
which we get from the tracer, store the trace.SpanContext as is and
explicitly extract the parent's span ID where necessary.

* Add ReadOnlySpan interface

Use this interface instead of export.SpanData in places where reading
information from a span is necessary. Use export.SpanData only when
exporting spans.

* Add ReadWriteSpan interface

Use this interface instead of export.SpanData in places where it is
necessary to read information from a span and write to it at the same
time.

* Rename export.SpanData to SpanSnapshot

SpanSnapshot represents the nature of this type as well as its
intended use more accurately.

Clarify the purpose of SpanSnapshot in the docs and emphasize what
should and should not be done with it.

* Rephrase attributesMap doc comment

"refreshes" is wrong for plural ("updates").

* Refactor span.End()

- Improve accuracy of span duration. Record span end time ASAP. We
  want to measure a user operation as accurately as possible, which
  means we want to mark the end time of a span as soon as possible
  after span.End() is called. Any operations we do inside span.End()
  before storing the end time affect the total duration of the span,
  and although these operations are rather fast at the moment they
  still seem to affect the duration of the span by "artificially"
  adding time between the start and end timestamps. This is relevant
  only in cases where the end time isn't explicitly specified.
- Remove redundant idempotence check. Now that IsRecording() is based
  on the value of span.endTime, IsRecording() will always return
  false after span.End() had been called because span.endTime won't
  be zero. This means we no longer need span.endOnce.
- Improve TestEndSpanTwice so that it also ensures subsequent calls
  to span.End() don't modify the span's end time.

* Update changelog

Co-authored-by: Tyler Yahn <codingalias@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

CHANGELOG.md

@@ -8,6 +8,20 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+
+- Add the `ReadOnlySpan` and `ReadWriteSpan` interfaces to provide better control for accessing span data. (#1360)
+
+### Changed
+
+- Rename `export.SpanData` to `export.SpanSnapshot` and use it only for exporting spans. (#1360)
+- Store the parent's full `SpanContext` rather than just its span ID in the `span` struct. (#1360)
+- Improve span duration accuracy. (#1360)
+
+### Removed
+
+- Remove `errUninitializedSpan` as its only usage is now obsolete. (#1360)
+
 ## [0.15.0] - 2020-12-10
 
 ### Added

exporters/otlp/internal/transform/span.go

@@ -28,8 +28,9 @@ const (
 	maxMessageEventsPerSpan = 128
 )
 
-// SpanData transforms a slice of SpanData into a slice of OTLP ResourceSpans.
-func SpanData(sdl []*export.SpanData) []*tracepb.ResourceSpans {
+// SpanData transforms a slice of SpanSnapshot into a slice of OTLP
+// ResourceSpans.
+func SpanData(sdl []*export.SpanSnapshot) []*tracepb.ResourceSpans {
 	if len(sdl) == 0 {
 		return nil
 	}
@@ -95,7 +96,7 @@ func SpanData(sdl []*export.SpanData) []*tracepb.ResourceSpans {
 }
 
 // span transforms a Span into an OTLP span.
-func span(sd *export.SpanData) *tracepb.Span {
+func span(sd *export.SpanSnapshot) *tracepb.Span {
 	if sd == nil {
 		return nil
 	}

exporters/otlp/internal/transform/span_test.go

@@ -199,7 +199,7 @@ func TestSpanData(t *testing.T) {
 	// March 31, 2020 5:01:26 1234nanos (UTC)
 	startTime := time.Unix(1585674086, 1234)
 	endTime := startTime.Add(10 * time.Second)
-	spanData := &export.SpanData{
+	spanData := &export.SpanSnapshot{
 		SpanContext: trace.SpanContext{
 			TraceID: trace.TraceID{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F},
 			SpanID:  trace.SpanID{0xFF, 0xFE, 0xFD, 0xFC, 0xFB, 0xFA, 0xF9, 0xF8},
@@ -279,7 +279,7 @@ func TestSpanData(t *testing.T) {
 		DroppedLinksCount:      3,
 	}
 
-	got := SpanData([]*export.SpanData{spanData})
+	got := SpanData([]*export.SpanSnapshot{spanData})
 	require.Len(t, got, 1)
 
 	assert.Equal(t, got[0].GetResource(), Resource(spanData.Resource))
@@ -296,7 +296,7 @@ func TestSpanData(t *testing.T) {
 
 // Empty parent span ID should be treated as root span.
 func TestRootSpanData(t *testing.T) {
-	sd := SpanData([]*export.SpanData{{}})
+	sd := SpanData([]*export.SpanSnapshot{{}})
 	require.Len(t, sd, 1)
 	rs := sd[0]
 	got := rs.GetInstrumentationLibrarySpans()[0].GetSpans()[0].GetParentSpanId()
@@ -306,5 +306,5 @@ func TestRootSpanData(t *testing.T) {
 }
 
 func TestSpanDataNilResource(t *testing.T) {
-	assert.NotPanics(t, func() { SpanData([]*export.SpanData{{}}) })
+	assert.NotPanics(t, func() { SpanData([]*export.SpanSnapshot{{}}) })
 }

exporters/otlp/otlp.go

@@ -197,20 +197,20 @@ func (e *Exporter) ExportKindFor(desc *metric.Descriptor, kind aggregation.Kind)
 	return e.exportKindSelector.ExportKindFor(desc, kind)
 }
 
-// ExportSpans exports a batch of SpanData.
-func (e *Exporter) ExportSpans(ctx context.Context, sds []*tracesdk.SpanData) error {
-	return e.uploadTraces(ctx, sds)
+// ExportSpans exports a batch of SpanSnapshot.
+func (e *Exporter) ExportSpans(ctx context.Context, ss []*tracesdk.SpanSnapshot) error {
+	return e.uploadTraces(ctx, ss)
 }
 
-func (e *Exporter) uploadTraces(ctx context.Context, sdl []*tracesdk.SpanData) error {
+func (e *Exporter) uploadTraces(ctx context.Context, ss []*tracesdk.SpanSnapshot) error {
 	ctx, cancel := e.cc.contextWithStop(ctx)
 	defer cancel()
 
 	if !e.cc.connected() {
 		return nil
 	}
 
-	protoSpans := transform.SpanData(sdl)
+	protoSpans := transform.SpanData(ss)
 	if len(protoSpans) == 0 {
 		return nil
 	}

exporters/otlp/otlp_integration_test.go

@@ -367,7 +367,7 @@ func TestNewExporter_collectorConnectionDiesThenReconnects(t *testing.T) {
 		// No endpoint up.
 		require.Error(
 			t,
-			exp.ExportSpans(ctx, []*exporttrace.SpanData{{Name: "in the midst"}}),
+			exp.ExportSpans(ctx, []*exporttrace.SpanSnapshot{{Name: "in the midst"}}),
 			"transport: Error while dialing dial tcp %s: connect: connection refused",
 			mc.address,
 		)
@@ -381,11 +381,11 @@ func TestNewExporter_collectorConnectionDiesThenReconnects(t *testing.T) {
 
 		n := 10
 		for i := 0; i < n; i++ {
-			require.NoError(t, exp.ExportSpans(ctx, []*exporttrace.SpanData{{Name: "Resurrected"}}))
+			require.NoError(t, exp.ExportSpans(ctx, []*exporttrace.SpanSnapshot{{Name: "Resurrected"}}))
 		}
 
 		nmaSpans := nmc.getSpans()
-		// Expecting 10 spanData that were sampled, given that
+		// Expecting 10 SpanSnapshots that were sampled, given that
 		if g, w := len(nmaSpans), n; g != w {
 			t.Fatalf("Round #%d: Connected collector: spans: got %d want %d", j, g, w)
 		}
@@ -461,7 +461,7 @@ func TestNewExporter_withHeaders(t *testing.T) {
 		otlp.WithAddress(mc.address),
 		otlp.WithHeaders(map[string]string{"header1": "value1"}),
 	)
-	require.NoError(t, exp.ExportSpans(ctx, []*exporttrace.SpanData{{Name: "in the midst"}}))
+	require.NoError(t, exp.ExportSpans(ctx, []*exporttrace.SpanSnapshot{{Name: "in the midst"}}))
 
 	defer func() {
 		_ = exp.Shutdown(ctx)

exporters/otlp/otlp_span_test.go

@@ -68,19 +68,19 @@ func TestExportSpans(t *testing.T) {
 	endTime := startTime.Add(10 * time.Second)
 
 	for _, test := range []struct {
-		sd   []*tracesdk.SpanData
+		sd   []*tracesdk.SpanSnapshot
 		want []tracepb.ResourceSpans
 	}{
 		{
-			[]*tracesdk.SpanData(nil),
+			[]*tracesdk.SpanSnapshot(nil),
 			[]tracepb.ResourceSpans(nil),
 		},
 		{
-			[]*tracesdk.SpanData{},
+			[]*tracesdk.SpanSnapshot{},
 			[]tracepb.ResourceSpans(nil),
 		},
 		{
-			[]*tracesdk.SpanData{
+			[]*tracesdk.SpanSnapshot{
 				{
 					SpanContext: trace.SpanContext{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}),

exporters/stdout/trace.go

@@ -31,19 +31,19 @@ type traceExporter struct {
 	stopped   bool
 }
 
-// ExportSpans writes SpanData in json format to stdout.
-func (e *traceExporter) ExportSpans(ctx context.Context, data []*trace.SpanData) error {
+// ExportSpans writes SpanSnapshots in json format to stdout.
+func (e *traceExporter) ExportSpans(ctx context.Context, ss []*trace.SpanSnapshot) error {
 	e.stoppedMu.RLock()
 	stopped := e.stopped
 	e.stoppedMu.RUnlock()
 	if stopped {
 		return nil
 	}
 
-	if e.config.DisableTraceExport || len(data) == 0 {
+	if e.config.DisableTraceExport || len(ss) == 0 {
 		return nil
 	}
-	out, err := e.marshal(data)
+	out, err := e.marshal(ss)
 	if err != nil {
 		return err
 	}

exporters/stdout/trace_test.go

@@ -46,7 +46,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 	doubleValue := 123.456
 	resource := resource.NewWithAttributes(label.String("rk1", "rv11"))
 
-	testSpan := &export.SpanData{
+	testSpan := &export.SpanSnapshot{
 		SpanContext: trace.SpanContext{
 			TraceID: traceID,
 			SpanID:  spanID,
@@ -67,7 +67,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 		StatusMessage: "interesting",
 		Resource:      resource,
 	}
-	if err := ex.ExportSpans(context.Background(), []*export.SpanData{testSpan}); err != nil {
+	if err := ex.ExportSpans(context.Background(), []*export.SpanSnapshot{testSpan}); err != nil {
 		t.Fatal(err)
 	}
 

exporters/trace/jaeger/jaeger.go

@@ -210,18 +210,18 @@ type Exporter struct {
 
 var _ export.SpanExporter = (*Exporter)(nil)
 
-// ExportSpans exports SpanData to Jaeger.
-func (e *Exporter) ExportSpans(ctx context.Context, spans []*export.SpanData) error {
+// ExportSpans exports SpanSnapshots to Jaeger.
+func (e *Exporter) ExportSpans(ctx context.Context, ss []*export.SpanSnapshot) error {
 	e.stoppedMu.RLock()
 	stopped := e.stopped
 	e.stoppedMu.RUnlock()
 	if stopped {
 		return nil
 	}
 
-	for _, span := range spans {
+	for _, span := range ss {
 		// TODO(jbd): Handle oversized bundlers.
-		err := e.bundler.Add(spanDataToThrift(span), 1)
+		err := e.bundler.Add(spanSnapshotToThrift(span), 1)
 		if err != nil {
 			return fmt.Errorf("failed to bundle %q: %w", span.Name, err)
 		}
@@ -260,9 +260,9 @@ func (e *Exporter) Shutdown(ctx context.Context) error {
 	return nil
 }
 
-func spanDataToThrift(data *export.SpanData) *gen.Span {
-	tags := make([]*gen.Tag, 0, len(data.Attributes))
-	for _, kv := range data.Attributes {
+func spanSnapshotToThrift(ss *export.SpanSnapshot) *gen.Span {
+	tags := make([]*gen.Tag, 0, len(ss.Attributes))
+	for _, kv := range ss.Attributes {
 		tag := keyValueToTag(kv)
 		if tag != nil {
 			tags = append(tags, tag)
@@ -273,34 +273,34 @@ func spanDataToThrift(data *export.SpanData) *gen.Span {
 	// semantic. Should resources be appended before span
 	// attributes, above, to allow span attributes to
 	// overwrite resource attributes?
-	if data.Resource != nil {
-		for iter := data.Resource.Iter(); iter.Next(); {
+	if ss.Resource != nil {
+		for iter := ss.Resource.Iter(); iter.Next(); {
 			if tag := keyValueToTag(iter.Attribute()); tag != nil {
 				tags = append(tags, tag)
 			}
 		}
 	}
-	if il := data.InstrumentationLibrary; il.Name != "" {
+	if il := ss.InstrumentationLibrary; il.Name != "" {
 		tags = append(tags, getStringTag(keyInstrumentationLibraryName, il.Name))
 		if il.Version != "" {
 			tags = append(tags, getStringTag(keyInstrumentationLibraryVersion, il.Version))
 		}
 	}
 
 	tags = append(tags,
-		getInt64Tag("status.code", int64(data.StatusCode)),
-		getStringTag("status.message", data.StatusMessage),
-		getStringTag("span.kind", data.SpanKind.String()),
+		getInt64Tag("status.code", int64(ss.StatusCode)),
+		getStringTag("status.message", ss.StatusMessage),
+		getStringTag("span.kind", ss.SpanKind.String()),
 	)
 
 	// Ensure that if Status.Code is not OK, that we set the "error" tag on the Jaeger span.
 	// See Issue https://github.com/census-instrumentation/opencensus-go/issues/1041
-	if data.StatusCode != codes.Ok && data.StatusCode != codes.Unset {
+	if ss.StatusCode != codes.Ok && ss.StatusCode != codes.Unset {
 		tags = append(tags, getBoolTag("error", true))
 	}
 
 	var logs []*gen.Log
-	for _, a := range data.MessageEvents {
+	for _, a := range ss.MessageEvents {
 		fields := make([]*gen.Tag, 0, len(a.Attributes))
 		for _, kv := range a.Attributes {
 			tag := keyValueToTag(kv)
@@ -316,7 +316,7 @@ func spanDataToThrift(data *export.SpanData) *gen.Span {
 	}
 
 	var refs []*gen.SpanRef
-	for _, link := range data.Links {
+	for _, link := range ss.Links {
 		refs = append(refs, &gen.SpanRef{
 			TraceIdHigh: int64(binary.BigEndian.Uint64(link.TraceID[0:8])),
 			TraceIdLow:  int64(binary.BigEndian.Uint64(link.TraceID[8:16])),
@@ -328,14 +328,14 @@ func spanDataToThrift(data *export.SpanData) *gen.Span {
 	}
 
 	return &gen.Span{
-		TraceIdHigh:   int64(binary.BigEndian.Uint64(data.SpanContext.TraceID[0:8])),
-		TraceIdLow:    int64(binary.BigEndian.Uint64(data.SpanContext.TraceID[8:16])),
-		SpanId:        int64(binary.BigEndian.Uint64(data.SpanContext.SpanID[:])),
-		ParentSpanId:  int64(binary.BigEndian.Uint64(data.ParentSpanID[:])),
-		OperationName: data.Name, // TODO: if span kind is added then add prefix "Sent"/"Recv"
-		Flags:         int32(data.SpanContext.TraceFlags),
-		StartTime:     data.StartTime.UnixNano() / 1000,
-		Duration:      data.EndTime.Sub(data.StartTime).Nanoseconds() / 1000,
+		TraceIdHigh:   int64(binary.BigEndian.Uint64(ss.SpanContext.TraceID[0:8])),
+		TraceIdLow:    int64(binary.BigEndian.Uint64(ss.SpanContext.TraceID[8:16])),
+		SpanId:        int64(binary.BigEndian.Uint64(ss.SpanContext.SpanID[:])),
+		ParentSpanId:  int64(binary.BigEndian.Uint64(ss.ParentSpanID[:])),
+		OperationName: ss.Name, // TODO: if span kind is added then add prefix "Sent"/"Recv"
+		Flags:         int32(ss.SpanContext.TraceFlags),
+		StartTime:     ss.StartTime.UnixNano() / 1000,
+		Duration:      ss.EndTime.Sub(ss.StartTime).Nanoseconds() / 1000,
 		Tags:          tags,
 		Logs:          logs,
 		References:    refs,

exporters/trace/jaeger/jaeger_test.go

@@ -353,7 +353,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 	assert.True(t, len(tc.spansUploaded) == 1)
 }
 
-func Test_spanDataToThrift(t *testing.T) {
+func Test_spanSnapshotToThrift(t *testing.T) {
 	now := time.Now()
 	traceID, _ := trace.TraceIDFromHex("0102030405060708090a0b0c0d0e0f10")
 	spanID, _ := trace.SpanIDFromHex("0102030405060708")
@@ -376,12 +376,12 @@ func Test_spanDataToThrift(t *testing.T) {
 
 	tests := []struct {
 		name string
-		data *export.SpanData
+		data *export.SpanSnapshot
 		want *gen.Span
 	}{
 		{
 			name: "no parent",
-			data: &export.SpanData{
+			data: &export.SpanSnapshot{
 				SpanContext: trace.SpanContext{
 					TraceID: traceID,
 					SpanID:  spanID,
@@ -465,7 +465,7 @@ func Test_spanDataToThrift(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := spanDataToThrift(tt.data)
+			got := spanSnapshotToThrift(tt.data)
 			sort.Slice(got.Tags, func(i, j int) bool {
 				return got.Tags[i].Key < got.Tags[j].Key
 			})

exporters/trace/zipkin/model.go

@@ -31,15 +31,15 @@ const (
 	keyInstrumentationLibraryVersion = "otel.instrumentation_library.version"
 )
 
-func toZipkinSpanModels(batch []*export.SpanData, serviceName string) []zkmodel.SpanModel {
+func toZipkinSpanModels(batch []*export.SpanSnapshot, serviceName string) []zkmodel.SpanModel {
 	models := make([]zkmodel.SpanModel, 0, len(batch))
 	for _, data := range batch {
 		models = append(models, toZipkinSpanModel(data, serviceName))
 	}
 	return models
 }
 
-func toZipkinSpanModel(data *export.SpanData, serviceName string) zkmodel.SpanModel {
+func toZipkinSpanModel(data *export.SpanSnapshot, serviceName string) zkmodel.SpanModel {
 	return zkmodel.SpanModel{
 		SpanContext: toZipkinSpanContext(data),
 		Name:        data.Name,
@@ -56,7 +56,7 @@ func toZipkinSpanModel(data *export.SpanData, serviceName string) zkmodel.SpanMo
 	}
 }
 
-func toZipkinSpanContext(data *export.SpanData) zkmodel.SpanContext {
+func toZipkinSpanContext(data *export.SpanSnapshot) zkmodel.SpanContext {
 	return zkmodel.SpanContext{
 		TraceID:  toZipkinTraceID(data.SpanContext.TraceID),
 		ID:       toZipkinID(data.SpanContext.SpanID),
@@ -145,7 +145,7 @@ var extraZipkinTags = []string{
 	keyInstrumentationLibraryVersion,
 }
 
-func toZipkinTags(data *export.SpanData) map[string]string {
+func toZipkinTags(data *export.SpanSnapshot) map[string]string {
 	m := make(map[string]string, len(data.Attributes)+len(extraZipkinTags))
 	for _, kv := range data.Attributes {
 		m[(string)(kv.Key)] = kv.Value.Emit()