Allow writes and read to not block SpanSinkImpl and only synchronize flush calls #547
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bidetofevil and the rest of your teammates on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #547 +/- ##
==========================================
+ Coverage 79.73% 80.12% +0.39%
==========================================
Files 409 412 +3
Lines 10938 10986 +48
Branches 1614 1617 +3
==========================================
+ Hits 8721 8803 +82
+ Misses 1569 1538 -31
+ Partials 648 645 -3
|
| return completedSpans.toList() | ||
| } | ||
| val spansToReturn = completedSpans.size | ||
| return completedSpans.take(spansToReturn) |
There was a problem hiding this comment.
Why is this needed? Instead of just returning completedSpans.toList()
There was a problem hiding this comment.
Because we're not synchronizing this and and write, we can have the completedSpans queue being modified while we are taking a snapshot. Just to be predictable especially for the flush use case, we are doing this so we aren't trying to flush events that were added while the flush was taking place.
Once the flush is triggered, everything that comes after should not be part of what's being flushed.
| synchronized(spansToFlush) { | ||
| spansToFlush.set(completedSpans()) | ||
| repeat(spansToFlush.get().size) { | ||
| completedSpans.removeAll(spansToFlush.get().toSet()) |
There was a problem hiding this comment.
Is this correct? removing all as many times as spans in the list?
There was a problem hiding this comment.
hahaha this is ABSOLUTELY not correct - I will change.
I had a previous implementation that polled N times to remove and add to the flushed spans, but that leaves the the object in a weird state if it crashes in the middle. Doing this atomically works much better even if I have to convert this to a set first.
bidetofevil
force-pushed
the
hho/improve-spanSink-concurrency
branch
from
a9c0107
to
f2bbde3
Compare
Goal
First step to persisting active spans is to sort out the concurrency story and ensuring that we don't lose data if the app crashes in a state where recorded spans are only in memory.
This change stores the flushed spans in a separate list that we can serialize before clearing the completedSpans queue.
Testing
Added test to test race condition caused by concurrent access.