Add RO/RW span interfaces #1360

johananl · 2020-11-20T18:41:56Z

General

This PR refactors the way we handle access to span data in the SDK.

Reviewing this PR commit by commit can be easier.

This PR touches code in the "critical path" of the SDK. I suggest we keep the following in mind when reviewing:

We should ensure we aren't making performance worse by merging this.
We should verify the set of methods under ReadOnlySpan make sense: Are we exposing everything which might need to be read? Are we exposing anything unnecessary?

Rationale

The spec defines multiple interfaces for interacting with spans. These interfaces make a clear distinction between the following:

Places in the codebase where a span should only be written to.
Places in the codebase where a span should only be read from.
Places in the codebase where a span might have to be either read from or written to.

This change improves the way we control access to the various fields of a span struct in the SDK.

In addition, this change explicitly designates export.SpanData as a read-only snapshot of a span. Following this change, export.SpanData instances are no longer modified after creation and are used solely for exporting spans.

Summary of changes

Store all span data directly in the span struct.
Add a ReadOnlySpan interface which matches the "readable span" concept from the spec.
Add a ReadWriteSpan interface which matches the "read/write span" concept from the spec.
Use the two interfaces above as appropriate instead of export.SpanData.

See the commit messages for more info about each change.

Testing

The easiest way to test this PR is to run the following unit tests:

cd sdk
go test -v ./trace -run TestReadOnlySpan
go test -v ./trace -run TestReadWriteSpan

codecov · 2020-11-20T18:45:23Z

Codecov Report

Merging #1360 (eea2c3f) into master (61e07a0) will increase coverage by 0.0%.
The diff coverage is 93.8%.

@@          Coverage Diff           @@
##           master   #1360   +/-   ##
======================================
  Coverage    77.9%   78.0%           
======================================
  Files         123     123           
  Lines        6202    6252   +50     
======================================
+ Hits         4836    4877   +41     
- Misses       1122    1126    +4     
- Partials      244     249    +5

Impacted Files	Coverage Δ
exporters/otlp/internal/transform/span.go	`98.0% <ø> (ø)`
exporters/trace/zipkin/model.go	`98.6% <ø> (ø)`
exporters/stdout/trace.go	`65.2% <50.0%> (ø)`
sdk/trace/simple_span_processor.go	`75.0% <66.6%> (+3.5%)`	⬆️
sdk/trace/span.go	`91.2% <93.5%> (-3.8%)`	⬇️
exporters/otlp/otlp.go	`83.5% <100.0%> (ø)`
exporters/trace/jaeger/jaeger.go	`84.6% <100.0%> (ø)`
exporters/trace/zipkin/zipkin.go	`73.6% <100.0%> (ø)`
sdk/export/trace/tracetest/test.go	`83.3% <100.0%> (ø)`
sdk/trace/attributesmap.go	`100.0% <100.0%> (ø)`
... and 4 more

sdk/trace/attributesmap.go

sdk/trace/span.go

sdk/trace/attributesmap.go

krnowak · 2020-11-23T16:40:43Z

The question that pops up is: Would it be ok to rename SpanData to something like SpanSnapshot? I realize that java implementation is calling it SpanData, but SpanSnapshot actually describes that thing better.

johananl · 2020-11-23T16:59:19Z

The question that pops up is: Would it be ok to rename SpanData to something like SpanSnapshot? I realize that java implementation is calling it SpanData, but SpanSnapshot actually describes that thing better.

I agree SpanSnapshot would make things less ambiguous. I'd let some more folks weigh in before changing this though.

Aneurysm9 · 2020-11-23T18:30:17Z

The question that pops up is: Would it be ok to rename SpanData to something like SpanSnapshot? I realize that java implementation is calling it SpanData, but SpanSnapshot actually describes that thing better.

I agree that SpanSnapshot more clearly articulates that this is a point-in-time snapshot of the data. I'd also suggest changing Dump() to Snapshot() to more clearly align the method name with the type it produces.

sdk/trace/span.go

johananl · 2020-11-24T15:29:07Z

Following the feedback above I've renamed SpanData to SpanSnapshot and span.Dump() to span.Snapshot().

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

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.

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

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.

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.

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

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

MrAlias · 2020-12-03T17:06:52Z

@jmacd I think all your concerns have been addressed, can you please take another look?

jmacd · 2020-12-11T00:47:54Z

Thanks @johananl

…/span-interfaces

MrAlias · 2020-12-11T03:27:58Z

Holding off on including this in the v0.15.0 release so we can hope to address the changes for the sdk/trace/export/trace package all in one release.

johananl requested review from Aneurysm9, evantorrie, jmacd, lizthegrey, MrAlias and XSAM as code owners November 20, 2020 18:41

johananl force-pushed the johananl/span-interfaces branch 3 times, most recently from 8dd2834 to 1f7baaf Compare November 20, 2020 19:19

Aneurysm9 approved these changes Nov 21, 2020

View reviewed changes

sdk/trace/attributesmap.go Show resolved Hide resolved

sdk/trace/span.go Outdated Show resolved Hide resolved

XSAM reviewed Nov 22, 2020

View reviewed changes

sdk/trace/attributesmap.go Outdated Show resolved Hide resolved

johananl force-pushed the johananl/span-interfaces branch 3 times, most recently from 48ecb63 to b7a4b05 Compare November 23, 2020 15:49

johananl force-pushed the johananl/span-interfaces branch from b7a4b05 to 0f8bbb7 Compare November 23, 2020 16:50

johananl force-pushed the johananl/span-interfaces branch from 0f8bbb7 to f78ed31 Compare November 23, 2020 19:18

jmacd requested changes Nov 24, 2020

View reviewed changes

sdk/trace/span.go Outdated Show resolved Hide resolved

sdk/trace/span.go Show resolved Hide resolved

johananl force-pushed the johananl/span-interfaces branch 8 times, most recently from 405c468 to bd9bdfd Compare November 24, 2020 15:28

johananl force-pushed the johananl/span-interfaces branch from c96737b to efbf014 Compare December 2, 2020 18:45

johananl added 8 commits December 2, 2020 19:46

Add ReadOnlySpan interface

3d43796

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

a469064

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

89c4c7c

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

aeaf929

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

Update changelog

0745b4f

johananl force-pushed the johananl/span-interfaces branch from efbf014 to 0745b4f Compare December 2, 2020 18:46

johananl requested a review from MrAlias December 3, 2020 16:26

MrAlias approved these changes Dec 3, 2020

View reviewed changes

Aneurysm9 approved these changes Dec 10, 2020

View reviewed changes

jmacd approved these changes Dec 11, 2020

View reviewed changes

MrAlias force-pushed the johananl/span-interfaces branch from 3566dff to 0745b4f Compare December 11, 2020 02:48

Merge remote-tracking branch 'upstream/master' into johananl-johananl…

2b66366

…/span-interfaces

XSAM approved these changes Dec 11, 2020

View reviewed changes

Merge branch 'master' into johananl/span-interfaces

eea2c3f

MrAlias merged commit c3c4273 into open-telemetry:master Dec 11, 2020

evantorrie mentioned this pull request Jan 7, 2021

UpdateName doesn't trigger sampling decision if the span is not sampled when created. #224

Closed

Aneurysm9 mentioned this pull request Jan 14, 2021

Prepare release v0.16.0 #1464

Merged

This was referenced Feb 17, 2021

SDK span no modification after ended #1543

Merged

SDK Span updates should be ignored after End is called #1313

Closed

johananl deleted the johananl/span-interfaces branch January 27, 2023 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RO/RW span interfaces #1360

Add RO/RW span interfaces #1360

johananl commented Nov 20, 2020

codecov bot commented Nov 20, 2020 •

edited

Loading

krnowak commented Nov 23, 2020

johananl commented Nov 23, 2020

Aneurysm9 commented Nov 23, 2020

johananl commented Nov 24, 2020

MrAlias commented Dec 3, 2020

jmacd commented Dec 11, 2020

MrAlias commented Dec 11, 2020

Add RO/RW span interfaces #1360

Add RO/RW span interfaces #1360

Conversation

johananl commented Nov 20, 2020

General

Rationale

Summary of changes

Testing

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

krnowak commented Nov 23, 2020

johananl commented Nov 23, 2020

Aneurysm9 commented Nov 23, 2020

johananl commented Nov 24, 2020

MrAlias commented Dec 3, 2020

jmacd commented Dec 11, 2020

MrAlias commented Dec 11, 2020

codecov bot commented Nov 20, 2020 •

edited

Loading