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

Fix metric SDK race condition #293

Merged
merged 3 commits into from
Nov 6, 2019
Merged

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Nov 6, 2019

Discovered while testing and documenting #265, this race condition between LoadOrStore and the assignment rec.recorder = i.meter.exporter.AggregatorFor(rec) could have resulted in lost updates. Fixing this would require two allocations prior to the LoadOrStore. Instead, a first call to Load checks for an existing record, followed by the LoadOrStore case. This results in fewer allocations overall, as shown in the updated benchmark.

Before:

BenchmarkAcquireNewHandle-8               	 1000000	      1192 ns/op	     275 B/op	       5 allocs/op
BenchmarkAcquireExistingHandle-8          	 2201809	       612 ns/op	     112 B/op	       2 allocs/op
BenchmarkAcquireReleaseExistingHandle-8   	 2219748	       590 ns/op	     112 B/op	       2 allocs/op

After:

BenchmarkAcquireNewHandle-8               	 1194990	      1157 ns/op	     255 B/op	       5 allocs/op
BenchmarkAcquireExistingHandle-8          	 5107620	       248 ns/op	       0 B/op	       0 allocs/op
BenchmarkAcquireReleaseExistingHandle-8   	 5356180	       253 ns/op	       0 B/op	       0 allocs/op

@jmacd jmacd merged commit 2546646 into open-telemetry:master Nov 6, 2019
@jmacd jmacd deleted the jmacd/fixrace branch November 6, 2019 18:54
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

3 participants