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

Additional context protections #544

Merged
merged 6 commits into from Oct 14, 2020

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Oct 14, 2020

Which problem is this PR solving?

Short description of the changes

  • Made isWriteable() and isSamplingFinalized() methods on SpanContext
  • Changed setTagInternal() and applySamplingDecision() to retrieve the context appropriately under lock or not
  • Changed setSamplingPriority() to take a *samplingState and other required params
  • Reenabled and extended TestSpanContextRaces()

Benchmarks

branch

goos: linux
goarch: amd64
pkg: github.com/uber/jaeger-client-go
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 4943131	       245 ns/op
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 3944086	       348 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 3023574	       400 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2553519	       472 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 1532324	       730 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 3645086	       337 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	  805288	      1443 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 1786104	       727 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8       	  650238	      1788 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8        	  619460	      1840 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8      	  594268	      1816 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8       	  561058	      1903 ns/op
PASS

master

goos: linux
goarch: amd64
pkg: github.com/uber/jaeger-client-go
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 6046756	       203 ns/op
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 4618400	       261 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 3357320	       361 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2852832	       422 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 1490359	       861 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 4337530	       283 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 1000000	      1064 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 1964678	       662 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8       	  753090	      1496 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8        	  763476	      1584 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8      	  735798	      1544 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8       	  674318	      1606 ns/op
PASS

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #544 into master will increase coverage by 0.05%.
The diff coverage is 92.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   88.46%   88.51%   +0.05%     
==========================================
  Files          61       61              
  Lines        3285     3301      +16     
==========================================
+ Hits         2906     2922      +16     
  Misses        252      252              
  Partials      127      127              
Impacted Files Coverage Δ
tracer.go 95.53% <66.66%> (+0.02%) ⬆️
span.go 97.04% <96.77%> (+0.18%) ⬆️
span_context.go 93.96% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a2c34e...1be7d73. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

As I understand this supersedes #528?

span.go Outdated Show resolved Hide resolved
tracer.go Outdated Show resolved Hide resolved
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

New benchmarks with additional protection in reportSpan():

goos: linux
goarch: amd64
pkg: github.com/uber/jaeger-client-go
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 4969408	       236 ns/op
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 3820220	       305 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 2593363	       400 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2531154	       456 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 1544758	       729 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 3588688	       340 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 1000000	      1374 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 1494066	       756 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8       	  605941	      1830 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8        	  599637	      1904 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8      	  587407	      1871 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8       	  756658	      1432 ns/op
PASS

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro merged commit ca99600 into jaegertracing:master Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DATA RACE : span.context overwritten in span.SetBaggageItem() without any guards
2 participants