Fix client/server span classification #102

vprithvi · 2016-12-01T18:17:06Z

The Span object has custom logic to set Span#isRpc and Span#isClient based
on the tags passed into Span#setTag.
Creating a Span using SpanBuilder bypasses this logic causing Span#isRPC to be
false. This causes ThriftSpanConverter to use lc annotations instead of sr/ss

This commit sanitizes input from SpanBuilder so that setting tags on the
SpanBuilder results in the same behavior as setting a tag on the Span.

The Span object has custom logic to set `Span#isRpc` and `Span#isClient` based on the tags passed into `Span#setTag`. Creating a Span using SpanBuilder bypasses this logic causing `Span#isRPC` to be false. This causes ThriftSpanConverter to use lc annotations instead of sr/ss This commit sanitizes input from SpanBuilder so that setting tags on the SpanBuilder results in the same behavior as setting a tag on the Span

coveralls · 2016-12-01T18:26:05Z

Coverage remained the same at 78.704% when pulling bbfa3eb on fix_lc_traces into 2bcca42 on master.

yurishkuro · 2016-12-01T18:32:12Z

jaeger-core/src/test/java/com/uber/jaeger/TracerTest.java

+  public void testBuildServerSpan() {
+    Span span = (com.uber.jaeger.Span) tracer.buildSpan("flexo")
+                                             .withTag(Tags.SPAN_KIND.getKey(),
+                                                      Tags.SPAN_KIND_SERVER).start();


odd formatting

Will update

badiib · 2016-12-01T18:32:40Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+  private void sanitizeAndSetTags(Map<String, Object> tags) {
+    this.tags = new HashMap<>();
+    for (Map.Entry<String, Object> tag : tags.entrySet()) {
+      setTagAsObject(tag.getKey(), tag.getValue());


a little worried about performance here - setTagAsObject locks every time. if sanitizeAndSetTags is only called in the constructor, I don't think we need to worry about syncing.

sanitizeAndSetTags is only called in the constructor, are you suggesting that we create a setTagAsObject that is not synchronized? Also, the average number of tags passed in are pretty small (<8) and this operation is only performed for spans that are sampled.

I would suggest not to synchronize ever where we need don't need to synchronize.

Agree, updated.

yurishkuro · 2016-12-01T18:33:47Z

jaeger-core/src/test/java/com/uber/jaeger/TracerTest.java

+  }
+
+  @Test
+  public void testBuildClientSpan() {


this test seems identical to the one above

Good catch, I'll update

yurishkuro · 2016-12-01T18:34:54Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+  /**
+   * uses {@link #setTagAsObject} to only insert tags that don't have special significance
+   */
+  private void sanitizeAndSetTags(Map<String, Object> tags) {


"sanitize" implies validation and potentially discarding or replacing tags, we do not do that here.

That's incorrect, we check and discard tags.
For e.g., one can never set a span.kind:server on the Span object. It is always discarded after setting the isRPC field to true. Other tags that are discarded are PEER_PORT, PEER_HOST, PEER_SERVICE, etc.

one can never set a span.kind:server on the Span object. It is always discarded after setting the isRPC field to true.

This method is called from the constructor, so "after" is not applicable. And we do not discard any tags; they may not be stored as tags explicitly, but they are certainly "stored" one way or another. Nothing is being "sanitized" here.

I would just inline this code in the constructor, since it's never reused. It would also allow tags member variable to be final

Also, once we upgrade this to jaeger.thrift, all this special handling goes away.

And we do not discard any tags; they may not be stored as tags explicitly, but they are certainly "stored" one way or another. Nothing is being "sanitized" here.

I agree that the data contained in the tags are stored one way or the other.
The tags themselves are discarded and cannot be retrieved by Span#getTags. Generally, if something is not discarded, I expect something set using the setter to be retrievable using a getter.

In the class, setting tags using #setTag does not follow this behavior.

At any rate, I don't see the point of arguing about semantics here. I'll inline this code.

coveralls · 2016-12-01T19:04:53Z

Coverage remained the same at 78.704% when pulling abbc9c2 on fix_lc_traces into 2bcca42 on master.

coveralls · 2016-12-01T19:38:06Z

Coverage remained the same at 78.704% when pulling a7b9cd3 on fix_lc_traces into 2bcca42 on master.

yurishkuro · 2016-12-02T01:07:13Z

jaeger-core/src/test/java/com/uber/jaeger/TracerTest.java

@@ -66,6 +68,26 @@ public void testBuildSpan() {
  }

  @Test
+  public void testBuildServerSpan() {
+    Span span = (com.uber.jaeger.Span) tracer.buildSpan("flexo")
+                                             .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER)


can you put tracer.buildSpan("flexo") on the 2nd line to avoid the insane indenting by the formatter? Or at least put .buildSpan() on the second line.

This is what it looks like on my IDE, I think it is reasonable indenting. Putting .buildSpan on the next line would simply move the block to the left by a few characters. Could you elaborate on why the current indenting is insane? Perhaps github is rendering things differently?

coveralls · 2016-12-02T02:28:27Z