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

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

lambcode commented on issue #920: brave.flush annotation reported for already finished span
URL: https://github.com/apache/incubator-zipkin-brave/issues/920#issuecomment-500608375
 
 
   I've gone ahead and opened a pull requests adding the tests directly to the brave project. Let me know if you want to see any changes there. As you will be able to see, there are two different test cases that were added.
   
   To answer some of your questions:
   > can you clarify if this is only accessing the span which causes the flush
   
   Yes. The "brave.flush" annotation will be reported if the span is accessed. Further actions such as adding additional annotations or tags can be done, but do not prevent the flush from being reported.
   
   > Perhaps an example json would be helpful.
   
   The unit test will actually print out the json sent to zipkin, but here it is for convienence:
   ```
   {"traceId":"8e60f8cf66329c9d","id":"8e60f8cf66329c9d","timestamp":1560202120031424,"duration":7,"localEndpoint":{"serviceName":"my-service","ipv4":"127.0.0.1"}},
   {"traceId":"8e60f8cf66329c9d","id":"8e60f8cf66329c9d","localEndpoint":{"serviceName":"my-service","ipv4":"127.0.0.1"},"annotations":[{"timestamp":1560202120247816,"value":"brave.flush"}]}
   ```
   
   > 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.
   
   I'm not convinced this would work because additional annotations and tags can be added. I don't know what would make a "real" span look different from one of these spans that we don't want to report. A "real" span with a "brave.flush" annotation is definitely valid.
   
   --
   I've done some additional poking around in the brave source, and still think that my original proposal has some merit. I don't think it will necessarily have the memory cost that you may be thinking of. We would not need to track "all completed traces". We'd only have to track those `PendingSpan` instances created by `TraceContext` instances that have not been garbage collected yet. Once the `TraceContext` that created the `PendingSpan` has been GC'd we no longer need to track that `PendingSpan`.
   
   I've got a working proof of concept of this idea and would like to share this code as well to further this discussion, but am not sure whether I should include it on the same pull request, or open another one. Please let me know which you would prefer.
   
   The big caveat here is that my code only fixes the case where a single `Tracer` is used. The test case for multiple `Tracers` will still cause problems, so we'll need more discussion on that point.
   
   

----------------------------------------------------------------
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