Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor SDK span creation and implementation #2213

Merged
merged 7 commits into from Sep 2, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 30, 2021

The span creation and configuration process is split across the tracer Start method and the startSpanInternal function, each living in different files. This adds confusion when developing. It requires the developer to remember certain parts of the configuration happen in one place or the other. Additionally, when it is determined that a span is not recorded during its creation it still is implemented by a full functioning Span implementation. This adds unneeded memory overhead. These changes unify the creation and configuration of a new span and add a new nonRecordingSpan type.

Unification of the span creation in a newSpan method on the tracer inherently associates the tracer with the span creation and centralizes all the configuration needed for a new span. This creation path is split in the newSpan method. One for a non-recording and another for a recording span.

Benchmarks

Benchmarks performed in the otel/sdk/trace package.

Before

$ go test -bench=.
go.opentelemetry.io/otel/sdk/trace.recordStackTrace(...)
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkAttributesMapToKeyValue-8     	  368532	      3135 ns/op
BenchmarkStartEndSpan/AlwaysSample-8   	 1372064	       872.5 ns/op	     848 B/op	       9 allocs/op
BenchmarkStartEndSpan/NeverSample-8    	 1588966	       741.9 ns/op	     752 B/op	       8 allocs/op
BenchmarkSpanWithAttributes_4/AlwaysSample-8         	  714387	      1647 ns/op	    1584 B/op	      17 allocs/op
BenchmarkSpanWithAttributes_4/NeverSample-8          	 1367698	       877.7 ns/op	     944 B/op	       9 allocs/op
BenchmarkSpanWithAttributes_8/AlwaysSample-8         	  551848	      2054 ns/op	    2112 B/op	      23 allocs/op
BenchmarkSpanWithAttributes_8/NeverSample-8          	 1257816	       953.5 ns/op	    1136 B/op	       9 allocs/op
BenchmarkSpanWithAttributes_all/AlwaysSample-8       	  557142	      1965 ns/op	    1936 B/op	      21 allocs/op
BenchmarkSpanWithAttributes_all/NeverSample-8        	 1216846	       931.6 ns/op	    1072 B/op	       9 allocs/op
BenchmarkSpanWithAttributes_all_2x/AlwaysSample-8    	  323812	      3457 ns/op	    3236 B/op	      32 allocs/op
BenchmarkSpanWithAttributes_all_2x/NeverSample-8     	 1000000	      1177 ns/op	    1392 B/op	       9 allocs/op
BenchmarkTraceID_DotString-8                         	14290009	        80.33 ns/op
BenchmarkSpanID_DotString-8                          	19520773	        59.88 ns/op
PASS

After

$ go test -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkAttributesMapToKeyValue-8     	  382208	      2932 ns/op
BenchmarkStartEndSpan/AlwaysSample-8   	 1284544	       911.1 ns/op	     848 B/op	       9 allocs/op
BenchmarkStartEndSpan/NeverSample-8    	 3394278	       341.1 ns/op	     224 B/op	       3 allocs/op
BenchmarkSpanWithAttributes_4/AlwaysSample-8         	  690414	      1576 ns/op	    1584 B/op	      17 allocs/op
BenchmarkSpanWithAttributes_4/NeverSample-8          	 2494444	       474.5 ns/op	     416 B/op	       4 allocs/op
BenchmarkSpanWithAttributes_8/AlwaysSample-8         	  495074	      2243 ns/op	    2112 B/op	      23 allocs/op
BenchmarkSpanWithAttributes_8/NeverSample-8          	 2070771	       569.0 ns/op	     608 B/op	       4 allocs/op
BenchmarkSpanWithAttributes_all/AlwaysSample-8       	  550532	      2135 ns/op	    1936 B/op	      21 allocs/op
BenchmarkSpanWithAttributes_all/NeverSample-8        	 2279611	       539.2 ns/op	     544 B/op	       4 allocs/op
BenchmarkSpanWithAttributes_all_2x/AlwaysSample-8    	  334038	      3591 ns/op	    3236 B/op	      32 allocs/op
BenchmarkSpanWithAttributes_all_2x/NeverSample-8     	 1617702	       742.9 ns/op	     864 B/op	       4 allocs/op
BenchmarkTraceID_DotString-8                         	13296470	        80.64 ns/op
BenchmarkSpanID_DotString-8                          	19072332	        62.09 ns/op
PASS

The span creation and configuration process is split across the tracer
Start method and the startSpanInternal function, each living in
different files. This adds confusion when developing. It requires the
developer to remember certain parts of the configuration happen in one
place or the other.

This unifies the creation and configuration of a new span. It makes this
unification by creating a newSpan method on the tracer. This method will
do all the configuration needed for a new span other than setting the
execution task tracker. This remains a part of the Start method to
preserve any existing timing and tracing the already exists.
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:trace Part of OpenTelemetry tracing Skip Changelog PRs that do not require a CHANGELOG.md entry labels Aug 30, 2021
@MrAlias MrAlias marked this pull request as draft August 30, 2021 17:27
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #2213 (da18bb1) into main (db317fc) will decrease coverage by 0.1%.
The diff coverage is 90.1%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2213     +/-   ##
=======================================
- Coverage   72.8%   72.7%   -0.2%     
=======================================
  Files        176     176             
  Lines      12162   12178     +16     
=======================================
- Hits        8866    8863      -3     
- Misses      3057    3072     +15     
- Partials     239     243      +4     
Impacted Files Coverage Δ
sdk/trace/span.go 81.1% <75.4%> (-9.0%) ⬇️
sdk/trace/tracer.go 100.0% <100.0%> (+6.8%) ⬆️
exporters/jaeger/jaeger.go 94.3% <0.0%> (+0.8%) ⬆️

@MrAlias MrAlias changed the title Refactor startSpanInternal into a tracer method Refactor SDK span creation and implementation Aug 30, 2021
@MrAlias MrAlias marked this pull request as ready for review August 30, 2021 20:51
sdk/trace/span.go Show resolved Hide resolved
sdk/trace/tracer.go Outdated Show resolved Hide resolved
Copy link

@tobert tobert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking something up for otel-cli and happened to see this PR and read it over. I didn't see any mistakes, and having read this code on and off for the last few months, I really like how much clearer the code path is this way and this feels like a good change.

@jmacd
Copy link
Contributor

jmacd commented Aug 31, 2021

(This is a nice optimization!)

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No further question. Looks good. 💯

@Aneurysm9 Aneurysm9 merged commit c7ae470 into open-telemetry:main Sep 2, 2021
@MrAlias MrAlias deleted the newSpan branch September 2, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants