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

Remove locking from Jaeger exporter shutdown/export #1807

Merged
merged 5 commits into from
Apr 17, 2021

Conversation

MrAlias
Copy link
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

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