AsyncReporter/SpanHandler: make queuedMaxBytes=0 disable pre-flight size checks #260
Conversation
| * Multi-producer, multi-consumer queue that could be bounded by count or/and size. | ||
| */ | ||
| interface BoundedQueue<S> extends SpanWithSizeConsumer<S> { | ||
| static <S> BoundedQueue<S> create(int maxSize, int maxBytes) { |
There was a problem hiding this comment.
@codefromthecrypt would really appreciate your early feedback here: I decided to branch off ByteBoundedQueue & SizeBoundedQueue instead of conditional logic in ByteBoundedQueue, the benchmarks show slightly better throughput (plus certainly less allocations since we don't need sizes buffer anymore).
Please let me know if that is aligned / unaligned with what you've been thinking (I have not removed any pre-flight size checks in AsyncReporter/SpanHandler yet, didn't want to go too far).
There was a problem hiding this comment.
I thought I made a comment that this sounds good, but I guess I didn't click? sounds good
benchmarks/src/main/java/zipkin2/reporter/internal/AsyncReporterBenchmarks.java
Outdated
Show resolved
Hide resolved
|
| * Multi-producer, multi-consumer queue that could be bounded by count or/and size. | ||
| */ | ||
| interface BoundedQueue<S> extends SpanWithSizeConsumer<S> { | ||
| static <S> BoundedQueue<S> create(int maxSize, int maxBytes) { |
There was a problem hiding this comment.
I thought I made a comment that this sounds good, but I guess I didn't click? sounds good
core/src/main/java/zipkin2/reporter/internal/SizeBoundedQueue.java
Outdated
Show resolved
Hide resolved
core/src/main/java/zipkin2/reporter/internal/SizeBoundedQueue.java
Outdated
Show resolved
Hide resolved
core/src/main/java/zipkin2/reporter/internal/SizeBoundedQueue.java
Outdated
Show resolved
Hide resolved
core/src/main/java/zipkin2/reporter/internal/AsyncReporter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/zipkin2/reporter/internal/CountBoundedQueue.java
Outdated
Show resolved
Hide resolved
|
So, to restate assumptions here. We are allowing queuedMaxBytes=0 as a way to avoid sizing the span (microsecond, could be much less) in the critical path. What maybe I wasn't clear about is that in a library as well used as this is, we really cannot rationalize removing checks about the safety of outbound messages.
We should also not interfere with metrics except the one which is about the current size of the queue in bytes, which is no longer possible to calculate. So, this means, the behaviour before and after should be the same otherwise, same metrics, same drop behavior on send.. just there may be a case where we enqueue something too big, and drop it later (on the flush/reporter thread). Let's do it right or don't do it. I already spent a lot of time with you on this, so can help contribute to your branch, if you are running out of steam. I'd prefer us not bring this issue into another week, if we can avoid it. |
Please feel free to push the changes (I am not running out of steam but there is limited time I have to dedicate working on this), I am planning to finalize the tests for |
|
I created a diff on another branch about the core code change I think is needed. Basically, this pulls the (count) bounded queue into an abstract class, used directly when the size bound is zero. It then overrides behaviour when there is a size bound. There is tension around guarding on message size. Instead of passing a lot of stuff to the queue types, I chose to use an anonymous subclass inside async reporter, as then it already has all the objects needed. you can decide to factor it otherwise, but I think it is picking poison anyway. this is just one type of poison which I think results in the least code to maintain. check me! Note: all your other fixes around bench being broken etc are still needed.. this was just a sketch to get us to a point where we still guard on size regardless of which thread is doing it. https://github.com/openzipkin/zipkin-reporter-java/compare/less-code |
Thanks for that 🥇 , I kept 2 classes (it is easier to test in isolation) and |
core/src/main/java/zipkin2/reporter/internal/CountBoundedQueue.java
Outdated
Show resolved
Hide resolved
|
ps after this change and before the next minor, we should default to queuedMaxBytes 0 I think. In doing that I think we can close #204 because we won't need to calculate 1pct of memory anymore. As a side note we can consider removing the memory bound on the next major version of reporter as I think it causes more problems than it solves in hindsight. That said, per #247 we wouldn't soon remove it.. just possibly we can deprecate the builder functions for removal in 4 if you agree. |
|
above I meant deprecating for removal setting queuedMaxBytes > 0. the other bounds still make sense, like max size per message. |
core/src/main/java/zipkin2/reporter/internal/CountBoundedQueue.java
Outdated
Show resolved
Hide resolved
|
oops sorry the other PR drifted you as you had fixed benchmarks here 😿 |
…ize checks Signed-off-by: Andriy Redko <[email protected]>
…ng them till flushing occurs Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
… propertly, added tests Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
|
@codefromthecrypt one last look? 🙏 |
Branched off
SizeBoundedQueueout ofByteBoundedQueuefor cases whenqueuedMaxBytes=0.