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

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Merged
merged 8 commits into from
Aug 20, 2018
Merged

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ad52db
Reduce visibility of TextMapCodec.contextFromString to package scope
Aug 17, 2018
a1cb911
Get rid of JaegerSpanContextTest
Aug 17, 2018
843c937
Remove outdated comment
Aug 17, 2018
8847020
Change contextAsString to package scope
Aug 20, 2018
fa973ea
Style fixes
Aug 20, 2018
986986e
Move contextAsString to public method of TextMapCodec
Aug 20, 2018
48112ed
Specify expected string output with string formatting
Aug 20, 2018
900524b
Use fixed IDs to avoid string formatting
Aug 20, 2018
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
2 changes: 1 addition & 1 deletion jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public String getBaggageItem(String key) {
@Override
public String toString() {
synchronized (this) {
return context.contextAsString() + " - " + operationName;
return context.toString() + " - " + operationName;
}
}

Expand Down
16 changes: 2 additions & 14 deletions jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@

package io.jaegertracing.internal;

import io.jaegertracing.internal.exceptions.EmptyTracerStateStringException;
import io.jaegertracing.internal.exceptions.MalformedTracerStateStringException;
import io.jaegertracing.internal.propagation.TextMapCodec;
import io.opentracing.SpanContext;
import java.math.BigInteger;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -92,19 +90,9 @@ public boolean isDebug() {
return (flags & flagDebug) == flagDebug;
}

public String contextAsString() {
int intFlag = flags & 0xFF;
return new StringBuilder()
.append(Long.toHexString(traceId)).append(":")
.append(Long.toHexString(spanId)).append(":")
.append(Long.toHexString(parentId)).append(":")
.append(Integer.toHexString(intFlag))
.toString();
}

@Override
public String toString() {
return contextAsString();
return TextMapCodec.contextAsString(this);
}

public JaegerSpanContext withBaggageItem(String key, String val) {
Expand Down
19 changes: 17 additions & 2 deletions jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private TextMapCodec(Builder builder) {
this.baggagePrefix = builder.baggagePrefix;
}

public static JaegerSpanContext contextFromString(String value)
static JaegerSpanContext contextFromString(String value)
throws MalformedTracerStateStringException, EmptyTracerStateStringException {
if (value == null || value.equals("")) {
throw new EmptyTracerStateStringException();
Expand All @@ -76,9 +76,24 @@ public static JaegerSpanContext contextFromString(String value)
new BigInteger(parts[3], 16).byteValue());
}

/**
* Encode context into a string.
* @param context Span context to encode.
* @return Encoded string representing span context.
*/
public static String contextAsString(JaegerSpanContext context) {
int intFlag = context.getFlags() & 0xFF;
return new StringBuilder()
.append(Long.toHexString(context.getTraceId())).append(":")
.append(Long.toHexString(context.getSpanId())).append(":")
.append(Long.toHexString(context.getParentId())).append(":")
.append(Integer.toHexString(intFlag))
.toString();
}

@Override
public void inject(JaegerSpanContext spanContext, TextMap carrier) {
carrier.put(contextKey, encodedValue(spanContext.contextAsString()));
carrier.put(contextKey, encodedValue(contextAsString(spanContext)));
for (Map.Entry<String, String> entry : spanContext.baggageItems()) {
carrier.put(keys.prefixedKey(entry.getKey(), baggagePrefix), encodedValue(entry.getValue()));
}
Expand Down
66 changes: 0 additions & 66 deletions jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanContextTest.java
View file
Open in desktop

This file was deleted.

16 changes: 6 additions & 10 deletions jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import io.jaegertracing.internal.clock.Clock;
import io.jaegertracing.internal.metrics.InMemoryMetricsFactory;
import io.jaegertracing.internal.metrics.Metrics;
import io.jaegertracing.internal.propagation.TextMapCodec;
import io.jaegertracing.internal.reporters.InMemoryReporter;
import io.jaegertracing.internal.samplers.ConstSampler;
import io.jaegertracing.spi.BaggageRestrictionManager;
Expand Down Expand Up @@ -200,7 +199,7 @@ public void testWithoutTimestampsInaccurateClock() {
.thenThrow(new IllegalStateException("currentTimeMicros() called 2nd time"));
when(clock.currentNanoTicks()).thenReturn(20000L).thenReturn(30000L);

JaegerSpan jaegerSpan = tracer.buildSpan("test-service-name").start();
JaegerSpan jaegerSpan = tracer.buildSpan("test-operation").start();
jaegerSpan.finish();

assertEquals(1, reporter.getSpans().size());
Expand All @@ -210,14 +209,11 @@ public void testWithoutTimestampsInaccurateClock() {

@Test
public void testSpanToString() {
JaegerSpan jaegerSpan = tracer.buildSpan("test-operation").start();
JaegerSpanContext expectedContext = jaegerSpan.context();
JaegerSpanContext actualContext = TextMapCodec.contextFromString(expectedContext.contextAsString());

assertEquals(expectedContext.getTraceId(), actualContext.getTraceId());
assertEquals(expectedContext.getSpanId(), actualContext.getSpanId());
assertEquals(expectedContext.getParentId(), actualContext.getParentId());
assertEquals(expectedContext.getFlags(), actualContext.getFlags());
Copy link
Member

Choose a reason for hiding this comment

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

removing it is not equivalent. But you can change L215 to use TextMapCodec's extract method.

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 am not sure if this still applies. See testSpanToString in TextMapCodec.

Copy link
Member

@yurishkuro yurishkuro Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here's what I am thinking. The method is called testSpanToString. Such method should belong in SpanTest class, not the codecs. But it's not actually testing span.toString(), only spanContext.contextAsString(). So let's separate these.

  • to test span.toString(), we can simply compare with an exact literal string (including the operation name).
  • what is the contract of contextAsString()? Do we, strictly speaking, care if the output matches the output of the codec? What if we use a different codec in the future? I think it mattered more when span context had the opposite fromString() method, but now contextAsString() seems merely a dependency of span.toString(), nothing more. Or is it used elsewhere? If not, we can not test it at all (since it's covered by span.toString() test), and we can consider making it package viz.

Counter-proposal: keep the test here (since it's testing Span's behavior, not the codec), but

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what? :)

Copy link
Member

Choose a reason for hiding this comment

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

sorry, a left over

String operation = "test-operation";
JaegerSpan span = tracer.buildSpan(operation).start();
String expectedString = span.context().toString() + " - " + operation;
Copy link
Member

Choose a reason for hiding this comment

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

prod == prod. Please test for exact string.

Copy link
Contributor Author

@isaachier isaachier Aug 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You mean I should use a string literal instead of expectedString? Not sure what the suggestion is.

Copy link
Member

Choose a reason for hiding this comment

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

yes, expectedString = "some literal value". Otherwise you are just comparing outputs of two prod functions. If we change the representation used by the codec, technically this test should fail since span.toString() would change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Will do.

assertEquals(expectedString, span.toString());
span.finish();
}

@Test
Expand Down
39 changes: 39 additions & 0 deletions jaeger-core/src/test/java/io/jaegertracing/internal/propagation/TextMapCodecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@

package io.jaegertracing.internal.propagation;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import io.jaegertracing.internal.JaegerSpanContext;
import io.jaegertracing.internal.exceptions.EmptyTracerStateStringException;
import io.jaegertracing.internal.exceptions.MalformedTracerStateStringException;
import org.junit.Test;

public class TextMapCodecTest {
Expand Down Expand Up @@ -44,4 +48,39 @@ public void testWithoutBuilder() {
assertTrue(str.contains("urlEncoding=false"));
}

@Test(expected = MalformedTracerStateStringException.class)
public void testContextFromStringMalformedException() throws Exception {
TextMapCodec.contextFromString("ff:ff:ff");
}

@Test(expected = EmptyTracerStateStringException.class)
public void testContextFromStringEmptyException() throws Exception {
TextMapCodec.contextFromString("");
}

@Test
public void testContextFromString() throws Exception {
JaegerSpanContext context = TextMapCodec.contextFromString("ff:dd:cc:4");
assertEquals(context.getTraceId(), 255);
assertEquals(context.getSpanId(), 221);
assertEquals(context.getParentId(), 204);
assertEquals(context.getFlags(), 4);
}

@Test
public void testContextAsStringFormatsPositiveFields() {
long traceId = -10L;
long spanId = -10L;
long parentId = -10L;
byte flags = (byte) 129;

JaegerSpanContext context = new JaegerSpanContext(traceId, spanId, parentId, flags);
assertEquals(
"fffffffffffffff6:fffffffffffffff6:fffffffffffffff6:81", TextMapCodec.contextAsString(context));
JaegerSpanContext contextFromStr = TextMapCodec.contextFromString(context.toString());
assertEquals(traceId, contextFromStr.getTraceId());
assertEquals(spanId, contextFromStr.getSpanId());
assertEquals(parentId, contextFromStr.getParentId());
assertEquals(flags, contextFromStr.getFlags());
}
}