Add Process IP to jaeger.thrift #197

black-adder · 2017-06-19T14:29:54Z

No description provided.

vprithvi · 2017-06-19T14:32:54Z

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

    this.version = loadVersion();

    tags.put("jaeger.version", this.version);
    String hostname = getHostName();
    if (hostname != null) {
      tags.put("jaeger.hostname", hostname);
    }
+    int ipv4 = 0;
+    try {
+      tags.put("ip", InetAddress.getLocalHost().getHostAddress());


Use a standard tag like PEER_HOST_IPV4

Also, I'd recommend putting InetAddress.getLocalHost().getHostAddress() into a variable because the host address might change due to DHCP.

it's not a standard opentracing tag, I can add it to constants I guess with jaeger.version and jaeger.hostname

so keep it as a variable and only commit to the ip only when we report? We report once a second, is it really that bad if for 1 second we report the wrong ip?

We define tracer tags at initialization and never update them. If IP address changes, there's nothing we can do without refactoring. I don't think it's a big concerns.

yurishkuro · 2017-06-19T15:00:01Z

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

+  /**
+   * The name used to report host name of the process.
+   */
+  public static final String TRACER_HOSTNAME_TAG_KEY = "jaeger.hostname";


I think this should be just hostname. It needed to be jaeger.xxx when we were sending Zipkin format since it did not have the distinction between span and process tags.

Checked the backend zipkin to jaeger converter to make sure all is kosher, looks good. Making the change.

what we may want to do is when the tags are added to Zipkin spans, add a tracer. prefix to them

maybe a later PR? need to coordinate with a backend change

sure. Although I don't know why you need a backend change. The idea here is that when people use Jaeger backend, they should be using jaeger.thrift reporters (udp or http). Only when they use Zipkin backend they would use zipkin.thrift reporter, where we convert tracer tags into first-in-process span tags.

nvm, I thought we were considering when the user uses zipkin reporter and the jaeger backend and we need to convert from zipkin to jaeger. I'll make the change.

yurishkuro · 2017-06-19T15:24:07Z

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

-    rootTags.put("tracer.tag.num", 1);
+    rootTags.put("jaeger.version", SENTINEL);
+    rootTags.put("jaeger.hostname", SENTINEL);
+    rootTags.put("tracer.tag.str", SENTINEL);


Confused by this change. The SENTINEL comment says "sentinel value is used to mark tags that should not be present". So if you're setting tracer.tag.str, why would it not be present?

this is for jaeger.thrift, these tags will only be present in process.tags or else we'll be double reporting the same tags in both process.tags and tags.

Well, I think this whole test was meant for zipkin.thrift, because it had the logic of only reporting the process tags in the first-in-process span. You're essentially changing everything to SENTINEL, meaning these tags are not going to be set on the span.

I already have similar tests for this in zipkin.thrift. I'd argue this test still provides some value to ensure tags aren't double reported in jaeger.thrift; but I'll update this test to provide some better value (ie check the process.tags actually exist)

codecov-io · 2017-06-19T15:44:37Z

Codecov Report

Merging #197 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #197      +/-   ##
============================================
+ Coverage     81.31%   81.34%   +0.02%     
- Complexity      465      470       +5     
============================================
  Files            77       77              
  Lines          1804     1812       +8     
  Branches        211      214       +3     
============================================
+ Hits           1467     1474       +7     
  Misses          249      249              
- Partials         88       89       +1

Impacted Files	Coverage Δ	Complexity Δ
...-core/src/main/java/com/uber/jaeger/Constants.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ber/jaeger/senders/zipkin/ThriftSpanConverter.java	`85.05% <100%> (+1.72%)`	`25 <1> (+6)`	⬆️
...ger-core/src/main/java/com/uber/jaeger/Tracer.java	`87.72% <72.72%> (-0.06%)`	`17 <1> (ø)`
...jaeger/reporters/protocols/ThriftUdpTransport.java	`77.96% <0%> (-5.09%)`	`14% <0%> (-2%)`
...a/com/uber/jaeger/senders/zipkin/ZipkinSender.java	`68.25% <0%> (+3.17%)`	`15% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4752c93...1b3829e. Read the comment docs.

pavolloffay · 2017-06-19T15:55:35Z

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

@@ -397,11 +398,6 @@ private String debugId() {
        }
      }

-      // TODO move this to jaeger-zipkin, this adds tracer tags to zipkin first span in a process


Does this mean that tracer tags won't be added to Zipkin spans(root in a process)?

It's still added, but I've moved that logic into ZipkinSpanConverter as per the TODO

pavolloffay · 2017-06-19T16:09:03Z

jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverter.java

+      // add tracer tags to first zipkin span in a process but remove "ip" tag as it is
+      // taken care of separately.
+      for (String tagKey : span.getTracer().tags().keySet()) {
+        if (!tagKey.equals("ip")) {


nit TRACER_IP_TAG_KEY?

yurishkuro · 2017-06-19T16:13:05Z

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

+    int ipv4 = 0;
+    try {
+      tags.put(Constants.TRACER_IP_TAG_KEY, InetAddress.getLocalHost().getHostAddress());
+      ipv4 = Utils.ipToInt(Inet4Address.getLocalHost().getHostAddress());


DRY: define local var for InetAddress.getLocalHost().getHostAddress()

one is InetAddress and the other is Inet4Address

yurishkuro · 2017-06-19T16:13:41Z

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

    }
+    int ipv4 = 0;


remove default value since you're setting to 0 in catch

yurishkuro · 2017-06-19T16:23:12Z

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

+    Map<String, ?> processTags = tracer.tags();
+    assertEquals(true, processTags.containsKey("jaeger.version"));
+    assertEquals(true, processTags.containsKey("hostname"));
+    assertEquals(true, processTags.containsKey("tracer.tag.str"));


This test still doesn't make sense to me. The reason it was parameterized was to ensure that tracer tags are converted into span tags when the span is first-in-process (2 out of 3 parameterized test cases). Since that logic has moved to Zipkin converter, this test should be moved there and operate only on the converted Zipkin span. The test that exists there is not as comprehensive as this one.

We may add a separate test here that tracer-level tags do not make it to Span tags, as well as that withTag method of the TracerBuilder works (which probably exists already elsewhere).

yurishkuro · 2017-06-19T19:28:06Z

jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverter.java

+    boolean firstSpanInProcess = span.getReferences().isEmpty() || isRpcServer(span);
+
+    Map<String, ?> processTags = span.getTracer().tags();
+    if (firstSpanInProcess && processTags != null) {


tracer tags are never null: `this.tags = Collections.unmodifiableMap(tags);

yurishkuro · 2017-06-19T19:30:43Z

jaeger-zipkin/src/test/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverterTest.java

+  private static final String SENTINEL = "sentinel";
+
+  // NA value is used to mark tags that should be present but we don't care what it's value is
+  private static final String NA = "na";


yurishkuro · 2017-06-19T19:31:35Z

jaeger-zipkin/src/test/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverterTest.java

@@ -56,6 +64,105 @@ public void setUp() {
            .build();
  }

+  // sentinel value is used to mark tags that should *not* be present
+  private static final String SENTINEL = "sentinel";


maybe rename this to UNDEF, easier to understand/read

yurishkuro · 2017-06-19T19:32:36Z

jaeger-zipkin/src/test/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverterTest.java

+    for (String key : expectedTags.keySet()) {
+      Object expectedValue = expectedTags.get(key);
+      BinaryAnnotation anno = findBinaryAnnotation(annotations, key);
+      if (expectedValue == SENTINEL) {


compare strings with equal()

yurishkuro · 2017-06-19T19:34:10Z

jaeger-zipkin/src/test/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverterTest.java

    assertTrue(serverReceiveFound);
    assertTrue(serverSendFound);
  }

  @Test
  public void testSpanKindClientCreatesAnnotations() {
-    Span span = (com.uber.jaeger.Span) tracer.buildSpan("operation-name").startManual();
+    Span parent = (com.uber.jaeger.Span) tracer.buildSpan("operation-name").startManual();


why does this test require a parent?

it doesn't remnant of previous change, removing

yurishkuro · 2017-06-19T21:30:30Z

jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverter.java

@@ -88,6 +90,20 @@
    Map<String, Object> tags = span.getTags();
    boolean isRpc = isRpc(span);
    boolean isClient = isRpcClient(span);
+    boolean firstSpanInProcess = span.getReferences().isEmpty() || isRpcServer(span);
+
+    Map<String, ?> processTags = span.getTracer().tags();


scope this inside the if{}

Add Process IP to jaeger.thrift

84b5528

vprithvi reviewed Jun 19, 2017

View reviewed changes

address feedback

3d9bf26

yurishkuro reviewed Jun 19, 2017

View reviewed changes

black-adder added 2 commits June 19, 2017 11:32

address feedback

b546b85

moar value

a7f3d32

pavolloffay reviewed Jun 19, 2017

View reviewed changes

yurishkuro reviewed Jun 19, 2017

View reviewed changes

omg

e507dca

yurishkuro reviewed Jun 19, 2017

View reviewed changes

asdasdasfashjkfaskhgfkas bfkasfhas

0c424bf

yurishkuro approved these changes Jun 19, 2017

View reviewed changes

last thing

1b3829e

black-adder merged commit 74faa98 into master Jun 19, 2017

black-adder deleted the set_process_ip branch June 19, 2017 22:15

mabn mentioned this pull request Jun 19, 2017

Utils.ipToInt is incorrect #198

Closed

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Process IP to jaeger.thrift #197

Add Process IP to jaeger.thrift #197

black-adder commented Jun 19, 2017

vprithvi Jun 19, 2017 •

edited

Loading

vprithvi Jun 19, 2017

black-adder Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

codecov-io commented Jun 19, 2017 •

edited

Loading

pavolloffay Jun 19, 2017

black-adder Jun 19, 2017

pavolloffay Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

yurishkuro Jun 19, 2017

black-adder Jun 19, 2017

yurishkuro Jun 19, 2017

Add Process IP to jaeger.thrift #197

Add Process IP to jaeger.thrift #197

Conversation

black-adder commented Jun 19, 2017

vprithvi Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vprithvi Jun 19, 2017 •

edited

Loading

codecov-io commented Jun 19, 2017 •

edited

Loading