zipkin-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [incubator-zipkin-brave] adriancole commented on a change in pull request #921: Add tests for #920. "brave.fush" annotation should not be reported for finished spans
Date Thu, 13 Jun 2019 00:11:12 GMT
adriancole commented on a change in pull request #921: Add tests for #920. "brave.fush" annotation
should not be reported for finished spans
URL: https://github.com/apache/incubator-zipkin-brave/pull/921#discussion_r293164584
 
 

 ##########
 File path: brave/src/test/java/brave/TracerTest.java
 ##########
 @@ -693,6 +695,52 @@
         .isPositive();
   }
 
+  @Test public void useSpanAfterFinished_doesNotCauseBraveFlush() throws InterruptedException
{
+    simulateInProcessPropagation(tracer, tracer);
+    blockOnGC();
+    tracer.newTrace().start().abandon(); //trigger orphaned span check
+    assertThat(spans).hasSize(1);
+    assertThat(spans.stream()
+      .flatMap(span -> span.annotations().stream())
+      .map(Annotation::value)
+      .collect(Collectors.toList())).doesNotContain("brave.flush");
+  }
+
+  @Test public void useSpanAfterFinishedInOtherTracer_doesNotCauseBraveFlush()
+    throws InterruptedException {
+    Tracer noOpTracer = Tracing.newBuilder()
+      .build().tracer();
+    simulateInProcessPropagation(noOpTracer, tracer);
+    blockOnGC();
+    tracer.newTrace().start().abandon(); //trigger orphaned span check
+
+    // We expect the span to be reported to the NOOP reporter, and nothing to be reported
to "spans"
+    assertThat(spans).hasSize(0);
+    assertThat(spans.stream()
+      .flatMap(span -> span.annotations().stream())
+      .map(Annotation::value)
+      .collect(Collectors.toList())).doesNotContain("brave.flush");
+  }
+
+  /**
+   * Must be a separate method from the test method to allow for local variables to be garbage
+   * collected
+   */
+  private static void simulateInProcessPropagation(Tracer tracer1, Tracer tracer2) {
 
 Review comment:
   I will try to answer your questions:
   
   Q: why let people make instances if should be a singleton?
   A: many frameworks handle singletons differently, and there are classloader concerns, too.
brave does not constrain you to using spring, guice, or otherwise singleton management.
   
   Q: should we be using different instances per endpoint in order to control the service
name?
   A: this is complex and likely more fine grained than most use. For most sites endpoints
are actually span names. We have a tool called FinishedSpanHandler which has the concept of
localRoot which would allow you to rewrite data consistently per fraction of a trace. Again,
this is more complex that what most sites do. if you want to discuss that further better use
chat as it is less spammy to watchers https://gitter.im/openzipkin/zipkin
   
   On the test talking about threads then not using them etc. I personally found this confusing
and it took me a few hours to guess what was supposed to be checked. (think time not pure
work). In general, being precise like your diagram is best as when comments lie they are distracting
to current and future maintainers. Your tests helped, just it mentioned things that wouldn't
have been tested in that code. thx regardless!

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


Mime
View raw message