Skip to content

Remove locking from Jaeger exporter shutdown/export#1807

Merged
MrAlias merged 5 commits intoopen-telemetry:mainfrom
MrAlias:jaeger-nolock
Apr 17, 2021
Merged

Remove locking from Jaeger exporter shutdown/export#1807
MrAlias merged 5 commits intoopen-telemetry:mainfrom
MrAlias:jaeger-nolock

Conversation

@MrAlias
Copy link
Copy Markdown
Contributor

@MrAlias MrAlias commented Apr 13, 2021

Replace the use of a lock to guard a state variable for shutdown of the Exporter with a channel. Additionally, hook this new channel into the context used during ExportSpans so if the exporter is shutdown in the middle of a call to this function it will cancel the context and flush.

Part of #1803 and needed for #1799.

@MrAlias MrAlias added Skip Changelog PRs that do not require a CHANGELOG.md entry pkg:exporter:jaeger Related to the Jaeger exporter package labels Apr 13, 2021
@MrAlias MrAlias added this to the RC1 milestone Apr 13, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2021

Codecov Report

Merging #1807 (7425cca) into main (4f9fec2) will decrease coverage by 0.0%.
The diff coverage is 80.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1807     +/-   ##
=======================================
- Coverage   78.4%   78.4%   -0.1%     
=======================================
  Files        135     135             
  Lines       7236    7249     +13     
=======================================
+ Hits        5679    5686      +7     
- Misses      1307    1313      +6     
  Partials     250     250             
Impacted Files Coverage Δ
exporters/trace/jaeger/jaeger.go 91.1% <80.0%> (-2.3%) ⬇️
sdk/trace/batch_span_processor.go 83.9% <0.0%> (ø)

@MrAlias
Copy link
Copy Markdown
Contributor Author

MrAlias commented Apr 13, 2021

Benchmark

From #1805 :

goos: linux                                                                  
goarch: amd64
pkg: go.opentelemetry.io/otel/exporters/trace/jaeger
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkCollectorExportSpans1-8       	  229094	      4475 ns/op	     800 B/op	      18 allocs/op
BenchmarkCollectorExportSpans10-8      	   88873	     14881 ns/op	    4737 B/op	      98 allocs/op
BenchmarkCollectorExportSpans100-8     	   14676	     79283 ns/op	   42874 B/op	     824 allocs/op
BenchmarkCollectorExportSpans1000-8    	    1704	    739067 ns/op	  417147 B/op	    8030 allocs/op
BenchmarkCollectorExportSpans10000-8   	     160	   7794122 ns/op	 4699637 B/op	   80050 allocs/op
BenchmarkAgentExportSpans1-8           	   10000	    104461 ns/op	    2088 B/op	      54 allocs/op
BenchmarkAgentExportSpans10-8          	   10000	    112531 ns/op	    6024 B/op	     135 allocs/op
BenchmarkAgentExportSpans100-8         	    4318	    276593 ns/op	   44168 B/op	     860 allocs/op

From this branch with a cherry-pick of that commit:

goos: linux                                                                                 
goarch: amd64
pkg: go.opentelemetry.io/otel/exporters/trace/jaeger
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkCollectorExportSpans1-8       	  249607	      4555 ns/op	     982 B/op	      21 allocs/op
BenchmarkCollectorExportSpans10-8      	   97167	     13130 ns/op	    4919 B/op	     101 allocs/op
BenchmarkCollectorExportSpans100-8     	   15291	     83771 ns/op	   43049 B/op	     827 allocs/op
BenchmarkCollectorExportSpans1000-8    	    1536	    690860 ns/op	  417324 B/op	    8033 allocs/op
BenchmarkCollectorExportSpans10000-8   	     154	   8055098 ns/op	 4699822 B/op	   80053 allocs/op
BenchmarkAgentExportSpans1-8           	   12698	     95846 ns/op	    2263 B/op	      57 allocs/op
BenchmarkAgentExportSpans10-8          	   10000	    113261 ns/op	    6200 B/op	     137 allocs/op
BenchmarkAgentExportSpans100-8         	    4179	    288180 ns/op	   44343 B/op	     863 allocs/op

This looks to add 3 allocs/op and not a definitive ns/op difference. This seems like a decent trade-off given the ability to hook into the context. Additionally, this (allocs and time) will be reduced much more in the ultimate destination of #1803.

@MrAlias MrAlias merged commit 2de86f2 into open-telemetry:main Apr 17, 2021
@MrAlias MrAlias deleted the jaeger-nolock branch April 17, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:exporter:jaeger Related to the Jaeger exporter 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.

3 participants