Rework span queued span completer#816
Conversation
The queued span completer was causing contention around the `Ref` for recording in flight requests. Seeing as this was only really used to ensure that the completer can be shut down properly, there's a better way of doing this new we have CE3.
- Use FS2 channels and their `close` semantics to back the `Queued` implementations - Make adding to the logging queue async, but putting `tryOffer` in a fire-and-forget fiber
| for { | ||
| channel <- Resource.eval(Channel.bounded[F, CompletedSpan](realBufferSize)) | ||
| errorQueue <- Resource.eval(Queue.bounded[F, Either[Channel.Closed, Boolean]](1)) | ||
| _ <- Stream |
There was a problem hiding this comment.
I don't think we wait for this stream to terminate, so we might never log that the Channel is closed. Is that a problem?
There was a problem hiding this comment.
Not really, I've just added the log for completeness
| exporter, | ||
| CompleterConfig(bufferSize = 50000, batchSize = 200) | ||
| ).use { completer => | ||
| List.fill(10000)(completer.complete(builder)).sequence |
There was a problem hiding this comment.
Is it worth doing something here to ensure that these are definitely not all handled in one groupWithin? Or a separate test for that? Of course it should all be uncancelable but it would be nice to be sure that the tests covered that
There was a problem hiding this comment.
They should all be handled in one groupWithin as the batchSize is just 200, the bufferSize here dictates how big the channel is, not the group.
There was a problem hiding this comment.
Just to be on the safe side, I'd suggest we might want to the number here to not be divided cleanly into batches (so 10001 for example). That way we verify that incomplete/partial batches are handled cleanly during shutdown
There was a problem hiding this comment.
Great idea, I'll change the tests
| ).use { completer => | ||
| val randomRequestTime = for { | ||
| rand <- random.betweenDouble(0, 1) | ||
| _ <- IO.sleep((1.second * rand).asInstanceOf[FiniteDuration]) |
There was a problem hiding this comment.
Is this just jitter so we don't hammer it with everything at once?
There was a problem hiding this comment.
Yep, I believe so, I think @sgjbryan wanted to improve this a bit
There was a problem hiding this comment.
I don't think we actually strictly need this jitter any more - was just part of me playing around to find a failing test for the old implementation. Expect this can be simplified a bit now
There was a problem hiding this comment.
sounds good, I just removed it
janstenpickle
requested review from
TimWSpence and
sam-tombury
and removed request for
TimWSpence and
sam-tombury
janstenpickle
force-pushed
the
queued-span-completer-contention
branch
from
4e2966b to
4542f08
Compare
TimWSpence
left a comment
TimWSpence
left a comment
There was a problem hiding this comment.
+1 on Sam's test suggestion but LGTM!
3 participants
The queued span completer was causing contention around the
Refforrecording in flight requests. Seeing as this was only really used to
ensure that the completer can be shut down properly, there's a better
way of doing this new we have CE3.