Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Add Process IP to jaeger.thrift #197

Merged
merged 7 commits into from
Jun 19, 2017
Merged

Add Process IP to jaeger.thrift #197

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
84b5528
Add Process IP to jaeger.thrift
black-adder Jun 19, 2017
3d9bf26
address feedback
black-adder Jun 19, 2017
b546b85
address feedback
black-adder Jun 19, 2017
a7f3d32
moar value
black-adder Jun 19, 2017
e507dca
omg
black-adder Jun 19, 2017
0c424bf
asdasdasfashjkfaskhgfkas bfkasfhas
black-adder Jun 19, 2017
1b3829e
last thing
black-adder Jun 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions jaeger-core/src/main/java/com/uber/jaeger/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,19 @@ public class Constants {
* root span, so that the trace can be found in the UI using this value as a correlation ID.
*/
public static final String DEBUG_ID_HEADER_KEY = "jaeger-debug-id";

/**
* The name of the tag used to report client version.
*/
public static final String JAEGER_CLIENT_VERSION_TAG_KEY = "jaeger.version";

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The name used to report ip of the process.
*/
public static final String TRACER_IP_TAG_KEY = "ip";
}
37 changes: 17 additions & 20 deletions jaeger-core/src/main/java/com/uber/jaeger/Tracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
import lombok.ToString;
import lombok.extern.slf4j.Slf4j;

import static com.uber.jaeger.Constants.JAEGER_CLIENT_VERSION_TAG_KEY;
import static com.uber.jaeger.Constants.TRACER_HOSTNAME_TAG_KEY;
import static com.uber.jaeger.Constants.TRACER_IP_TAG_KEY;

@ToString(exclude = {"registry", "clock", "metrics", "activeSpanSource"})
@Slf4j
public class Tracer implements io.opentracing.Tracer {
Expand All @@ -69,7 +73,7 @@ public class Tracer implements io.opentracing.Tracer {
private final PropagationRegistry registry;
private final Clock clock;
private final Metrics metrics;
private final int ip;
private final int ipv4;
private final Map<String, ?> tags;
private final boolean zipkinSharedRpcSpan;
private final ActiveSpanSource activeSpanSource;
Expand All @@ -93,21 +97,19 @@ private Tracer(
this.zipkinSharedRpcSpan = zipkinSharedRpcSpan;
this.activeSpanSource = activeSpanSource;

int ip;
try {
ip = Utils.ipToInt(Inet4Address.getLocalHost().getHostAddress());
} catch (UnknownHostException e) {
ip = 0;
}
this.ip = ip;

this.version = loadVersion();

tags.put("jaeger.version", this.version);
tags.put(JAEGER_CLIENT_VERSION_TAG_KEY, this.version);
String hostname = getHostName();
if (hostname != null) {
tags.put("jaeger.hostname", hostname);
tags.put(TRACER_HOSTNAME_TAG_KEY, hostname);
}
int ipv4 = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
tags.put(TRACER_IP_TAG_KEY, InetAddress.getLocalHost().getHostAddress());
ipv4 = Utils.ipToInt(Inet4Address.getLocalHost().getHostAddress());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one is InetAddress and the other is Inet4Address

} catch (UnknownHostException e) { }
this.ipv4 = ipv4;
this.tags = Collections.unmodifiableMap(tags);
}

Expand All @@ -127,8 +129,8 @@ public String getServiceName() {
return tags;
}

public int getIp() {
return ip;
public int getIpv4() {
return ipv4;
}

Clock clock() {
Expand Down Expand Up @@ -397,11 +399,6 @@ public io.opentracing.Span startManual() {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (zipkinSharedRpcSpan && (references.isEmpty() || isRpcServer())) {
tags.putAll(Tracer.this.tags);
}

Span span =
new Span(
Tracer.this,
Expand Down Expand Up @@ -557,15 +554,15 @@ private static String loadVersion() {
try {
Properties prop = new Properties();
prop.load(is);
version = prop.getProperty("jaeger.version");
version = prop.getProperty(JAEGER_CLIENT_VERSION_TAG_KEY);
} finally {
is.close();
}
} catch (Exception e) {
throw new RuntimeException("Cannot read jaeger.properties", e);
}
if (version == null) {
throw new RuntimeException("Cannot read jaeger.version from jaeger.properties");
throw new RuntimeException("Cannot read " + JAEGER_CLIENT_VERSION_TAG_KEY + " from jaeger.properties");
}
return "Java-" + version;
}
Expand Down
38 changes: 11 additions & 27 deletions jaeger-core/src/test/java/com/uber/jaeger/TracerTagsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,39 +61,25 @@ public TracerTagsTest(SpanType spanType, Map<String, Object> expectedTags) {
@Parameterized.Parameters(name = "{index}: {0}")
public static Collection<Object[]> data() {
Tracer tracer = new Tracer.Builder("x", null, null).build();
String hostname = tracer.getHostName();

Map<String, Object> rootTags = new HashMap<>();
rootTags.put("jaeger.version", tracer.getVersion());
rootTags.put("jaeger.hostname", hostname);
rootTags.put("tracer.tag.str", "y");
rootTags.put("tracer.tag.bool", true);
rootTags.put("tracer.tag.num", 1);
rootTags.put("jaeger.version", SENTINEL);
rootTags.put("jaeger.hostname", SENTINEL);
rootTags.put("tracer.tag.str", SENTINEL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

rootTags.put("sampler.type", "const");
rootTags.put("sampler.param", true);

Map<String, Object> childTags = new HashMap<>();
childTags.put("jaeger.version", SENTINEL);
childTags.put("jaeger.hostname", SENTINEL);
childTags.put("tracer.tag.str", SENTINEL);
childTags.put("tracer.tag.bool", SENTINEL);
childTags.put("tracer.tag.num", SENTINEL);
childTags.put("sampler.type", SENTINEL);
childTags.put("sampler.param", SENTINEL);

Map<String, Object> rpcTags = new HashMap<>();
rpcTags.put("jaeger.version", tracer.getVersion());
rpcTags.put("jaeger.hostname", hostname);
rpcTags.put("tracer.tag.str", "y");
rpcTags.put("tracer.tag.bool", true);
rpcTags.put("tracer.tag.num", 1);
rpcTags.put("sampler.type", SENTINEL);
rpcTags.put("sampler.param", SENTINEL);
Map<String, Object> sentinelTags = new HashMap<>();
sentinelTags.put("jaeger.version", SENTINEL);
sentinelTags.put("jaeger.hostname", SENTINEL);
sentinelTags.put("tracer.tag.str", SENTINEL);
sentinelTags.put("sampler.type", SENTINEL);
sentinelTags.put("sampler.param", SENTINEL);

List<Object[]> data = new ArrayList<>();
data.add(new Object[] {SpanType.ROOT, rootTags});
data.add(new Object[] {SpanType.CHILD, childTags});
data.add(new Object[] {SpanType.RPC_SERVER, rpcTags});
data.add(new Object[] {SpanType.CHILD, sentinelTags});
data.add(new Object[] {SpanType.RPC_SERVER, sentinelTags});
return data;
}

Expand All @@ -106,8 +92,6 @@ public void testTracerTagsZipkin() throws Exception {
Tracer tracer = new Tracer.Builder("x", spanReporter, new ConstSampler(true))
.withZipkinSharedRpcSpan()
.withTag("tracer.tag.str", "y")
.withTag("tracer.tag.bool", true)
.withTag("tracer.tag.num", 1)
.build();

Span span = (Span) tracer.buildSpan("root").startManual();
Expand Down
3 changes: 2 additions & 1 deletion jaeger-core/src/test/java/com/uber/jaeger/senders/UdpSenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,11 @@ public void testFlushSendsSpan() throws Exception {
assertEquals(0, actualSpan.getParentSpanId());
assertTrue(actualSpan.references.isEmpty());
assertEquals(expectedSpan.getOperationName(), actualSpan.getOperationName());
assertEquals(3, batch.getProcess().getTags().size());
assertEquals(4, batch.getProcess().getTags().size());
assertEquals("jaeger.hostname", batch.getProcess().getTags().get(0).getKey());
assertEquals("jaeger.version", batch.getProcess().getTags().get(1).getKey());
assertEquals("bar", batch.getProcess().getTags().get(2).getVStr());
assertEquals("ip", batch.getProcess().getTags().get(3).getKey());
}

@Test
Expand Down
23 changes: 20 additions & 3 deletions jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import io.opentracing.tag.Tags;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand All @@ -43,7 +44,7 @@ public class ThriftSpanConverter {

public static com.twitter.zipkin.thriftjava.Span convertSpan(Span span) {
Tracer tracer = span.getTracer();
Endpoint host = new Endpoint(tracer.getIp(), (short) 0, tracer.getServiceName());
Endpoint host = new Endpoint(tracer.getIpv4(), (short) 0, tracer.getServiceName());

SpanContext context = span.context();
return new com.twitter.zipkin.thriftjava.Span(
Expand Down Expand Up @@ -88,6 +89,18 @@ private static List<BinaryAnnotation> buildBinaryAnnotations(Span span, Endpoint
Map<String, Object> tags = span.getTags();
boolean isRpc = isRpc(span);
boolean isClient = isRpcClient(span);
boolean firstSpanInProcess = span.getReferences().isEmpty() || isRpcServer(span);

if (firstSpanInProcess && span.getTracer().tags() != null) {
// 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")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit TRACER_IP_TAG_KEY?

Object tagValue = tags.get(tagKey);
binaryAnnotations.add(buildBinaryAnnotation(tagKey, tagValue));
}
}
}

Endpoint peerEndpoint = extractPeerEndpoint(tags);
if (peerEndpoint != null && isClient) {
Expand Down Expand Up @@ -137,13 +150,17 @@ private static BinaryAnnotation buildBinaryAnnotation(String tagKey, Object tagV
return banno;
}

public static boolean isRpc(Span span) {
static boolean isRpcServer(Span span) {
return Tags.SPAN_KIND_SERVER.equals(span.getTags().get(Tags.SPAN_KIND.getKey()));
}

static boolean isRpc(Span span) {
Object spanKindValue = span.getTags().get(Tags.SPAN_KIND.getKey());
return Tags.SPAN_KIND_CLIENT.equals(spanKindValue) || Tags.SPAN_KIND_SERVER.equals(spanKindValue);

}

public static boolean isRpcClient(Span span) {
static boolean isRpcClient(Span span) {
return Tags.SPAN_KIND_CLIENT.equals(span.getTags().get(Tags.SPAN_KIND.getKey()));
}

Expand Down
40 changes: 32 additions & 8 deletions jaeger-zipkin/src/test/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static org.junit.Assert.assertFalse;

import com.twitter.zipkin.thriftjava.Annotation;
import com.twitter.zipkin.thriftjava.BinaryAnnotation;
import com.twitter.zipkin.thriftjava.zipkincoreConstants;
import com.uber.jaeger.Span;
import com.uber.jaeger.SpanContext;
Expand Down Expand Up @@ -70,19 +71,19 @@ public void testSpanKindServerCreatesAnnotations() {
if (anno.getValue().equals(zipkincoreConstants.SERVER_RECV)) {
serverReceiveFound = true;
}

if (anno.getValue().equals(zipkincoreConstants.SERVER_SEND)) {
serverSendFound = true;
}
}

assertTrue(serverReceiveFound);
assertTrue(serverSendFound);
findProcessTags(zipkinSpan, true);
}

@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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this test require a parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't remnant of previous change, removing

Span span = (com.uber.jaeger.Span) tracer.buildSpan("operation-name").asChildOf(parent).startManual();
Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT);

com.twitter.zipkin.thriftjava.Span zipkinSpan = ThriftSpanConverter.convertSpan(span);
Expand All @@ -102,18 +103,41 @@ public void testSpanKindClientCreatesAnnotations() {

assertTrue(clientReceiveFound);
assertTrue(clientSendFound);
findProcessTags(zipkinSpan, false);
}

public void findProcessTags(com.twitter.zipkin.thriftjava.Span zipkinSpan, boolean processTagsExist) {
List<BinaryAnnotation> bAnnotations = zipkinSpan.getBinary_annotations();
boolean jaegerVersionFound = false;
boolean jaegerHostnameFound = false;
boolean ipFound = false;
for (BinaryAnnotation anno : bAnnotations) {
if (anno.getKey().equals("jaeger.version")) {
jaegerVersionFound = true;
}
if (anno.getKey().equals("jaeger.hostname")) {
jaegerHostnameFound = true;
}
if (anno.getKey().equals("ip")) {
ipFound = true;
}
}

assertEquals(processTagsExist, jaegerVersionFound);
assertEquals(processTagsExist, jaegerHostnameFound);
assertFalse(ipFound); // the "ip" tag should be removed
}

@Test
public void testExpectdLocalComponentNameUsed() {
String expectedCompnentName = "local-name";
public void testExpectedLocalComponentNameUsed() {
String expectedComponentName = "local-name";
Span span = (com.uber.jaeger.Span) tracer.buildSpan("operation-name").startManual();
Tags.COMPONENT.set(span, expectedCompnentName);
Tags.COMPONENT.set(span, expectedComponentName);

com.twitter.zipkin.thriftjava.Span zipkinSpan = ThriftSpanConverter.convertSpan(span);
String actualComponent =
new String(zipkinSpan.getBinary_annotations().get(0).getValue(), StandardCharsets.UTF_8);
assertEquals(expectedCompnentName, actualComponent);
new String(zipkinSpan.getBinary_annotations().get(3).getValue(), StandardCharsets.UTF_8);
assertEquals(expectedComponentName, actualComponent);
}

@Test
Expand Down