[GitHub] [incubator-zipkin-brave] adriancole commented on issue #920: brave.flush annotation reported for already finished span

[GitBox <gi...@apache.org>]

adriancole commented on issue #920: brave.flush annotation reported for already finished span
URL: https://github.com/apache/incubator-zipkin-brave/issues/920#issuecomment-500078559
 
 
   can you clarify if this is only accessing the span which causes the flush?
   eg not adding data, simply accessing it? Perhaps an example json would be
   helpful.
   
   If it is as you say and there is no data except traceid/spanid and the
   flush annotation (quite possible) I would recommend comparison against a
   these fields, keep the logging statement, but not send to zipkin. This
   would be cheaper and less impact than trying to track all completed traces,
   though if we had to do the latter at some point, we could revisit.
   
   Regardless, can you make the tests with existing unit tests in this repo,
   as it is less work for us to evaluate and this would also be how a fix
   would occur.
   
   On Sat, Jun 8, 2019 at 2:06 AM lambcode <no...@github.com> wrote:
   
   > Problem
   >
   > Brave Version: 5.6.0
   >
   > I am encountering unexpected behavior when using Tracer.currentSpan() to
   > get a span that has previously been finished. It appears that calling
   > currentSpan() restarts the span to a degree in that the TraceContext is
   > re-registered with PendingSpans. This in turn causes a brave.flush
   > annotation to be reported when that TraceContext gets GC'd even though in
   > reality the span has already been finished.
   > [image: Untitled Diagram(2)]
   > <https://user-images.githubusercontent.com/5820125/59121923-9561a100-8916-11e9-982d-953b5337b597.png>
   >
   > This could probably be avoided by calling abandon() on the Span that is
   > returned from currentSpan(), but in our case sometimes this code lives in
   > libraries that are not aware whether or not the current span needs to be
   > abandoned() (it was finished previously) or if it will be finished later.
   >
   > This is also causing odd behavior in zipkin-ui (not lens) such as spans
   > going missing presumably because it doesn't expect multiple spans with the
   > same spanId that aren't shared spans. (For example I found one span had
   > three documents in elasticsearch: a client span, a servier span, and
   > another span with the brave.flush annotation)
   > Proposed Fix
   >
   > I wonder if instead of removing PendingSpan entries from the HashMap when
   > PendingSpans.remove() is invoked, these entries could be marked as finished
   > somehow. In this manner PendingSpans will not forget about a TraceContext
   > until it has been GC'd meaning that the context can never be registered
   > with pending spans more than once.
   >
   > If this approach is pursued, special care will need to be taken when
   > dealing with multiple Tracer instances, as each Tracer has it's own
   > instance of PendingSpans that needs to remain in sync.
   >
   > I have attached a sample project with unit tests for both a single tracer
   > and multiple tracer test that exhibits this behavior.
   > bugjar.zip
   > <https://github.com/apache/incubator-zipkin-brave/files/3267116/bugjar.zip>
   >
   > —
   > You are receiving this because you are subscribed to this thread.
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/incubator-zipkin-brave/issues/920?email_source=notifications&email_token=AAAPVV5CDRIV4WPSINLHZETPZKPRBA5CNFSM4HVZGKU2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GYJ4C5A>,
   > or mute the thread
   > <https://github.com/notifications/unsubscribe-auth/AAAPVVYLHDCOHFDRE42MR5LPZKPRBANCNFSM4HVZGKUQ>
   > .
   >
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@zipkin.apache.org
For additional commands, e-mail: dev-help@zipkin.apache.org