Reduce visibility of TextMapCodec.contextFromString to package scope #519

isaachier · 2018-08-17T17:34:01Z

Which problem is this PR solving?

Additional fix for Move internal method contextFromString out of JaegerSpanContext #518 to reduce visibility.

Short description of the changes

Change visibility.
Move all tests that rely on this method's functionality into the TextMapCodecTest class.

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro · 2018-08-17T17:38:56Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanContextTest.java

-    assertEquals(traceId, contextFromStr.getTraceId());
-    assertEquals(spanId, contextFromStr.getSpanId());
-    assertEquals(parentId, contextFromStr.getParentId());
-    assertEquals(flags, contextFromStr.getFlags());


don't see how removing this makes the test equivalent. But this whole test can move to text codec test, since it's not really testing the context toString

Right let me fix that.

yurishkuro · 2018-08-17T17:40:11Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanTest.java

-    assertEquals(expectedContext.getTraceId(), actualContext.getTraceId());
-    assertEquals(expectedContext.getSpanId(), actualContext.getSpanId());
-    assertEquals(expectedContext.getParentId(), actualContext.getParentId());
-    assertEquals(expectedContext.getFlags(), actualContext.getFlags());


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

I am not sure if this still applies. See testSpanToString in TextMapCodec.

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

But what? :)

sorry, a left over

codecov · 2018-08-17T17:40:51Z

Codecov Report

Merging #519 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #519     +/-   ##
===========================================
+ Coverage     88.26%   88.37%   +0.1%     
  Complexity      500      500             
===========================================
  Files            65       65             
  Lines          1849     1849             
  Branches        239      239             
===========================================
+ Hits           1632     1634      +2     
+ Misses          141      138      -3     
- Partials         76       77      +1

Impacted Files	Coverage Δ	Complexity Δ
...egertracing/internal/propagation/TextMapCodec.java	`90.58% <100%> (+0.84%)`	`22 <3> (+1)`	⬆️
...ain/java/io/jaegertracing/internal/JaegerSpan.java	`93.45% <100%> (ø)`	`47 <0> (ø)`	⬇️
...a/io/jaegertracing/internal/JaegerSpanContext.java	`90.62% <100%> (+0.88%)`	`22 <1> (ø)`	⬇️
...ing/internal/samplers/RemoteControlledSampler.java	`89.53% <0%> (-1.17%)`	`17% <0%> (-1%)`
...egertracing/internal/reporters/RemoteReporter.java	`85.71% <0%> (+2.38%)`	`7% <0%> (ø)`	⬇️

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 b6a824d...900524b. Read the comment docs.

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro · 2018-08-17T17:51:09Z

jaeger-core/src/test/java/io/jaegertracing/internal/propagation/TextMapCodecTest.java

+    byte flags = (byte) 129;
+
+    // I use MIN_VALUE because the most significant bit, and thats when
+    // we want to make sure the hex number is positive.


this comment looks like some old left-over. Doesn't seem to make any sense now.

Good catch.

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro · 2018-08-20T19:21:39Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java

@@ -78,7 +78,7 @@ public static JaegerSpanContext contextFromString(String value)

  @Override
  public void inject(JaegerSpanContext spanContext, TextMap carrier) {
-    carrier.put(contextKey, encodedValue(spanContext.contextAsString()));
+    carrier.put(contextKey, encodedValue(spanContext.toString()));


I don't think it's a good idea to rely on toString() for marshaling purposes.

Maybe do the same thing as with fromString, move contextAsString() from SpanContext to TextMapCodec, but keep it public so that SpanContext.toString() can call it.

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro · 2018-08-20T19:58:39Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanTest.java

-    assertEquals(expectedContext.getFlags(), actualContext.getFlags());
+    String operation = "test-operation";
+    JaegerSpan span = tracer.buildSpan(operation).start();
+    String expectedString = span.context().toString() + " - " + operation;


prod == prod. Please test for exact string.

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

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.

Right. Will do.

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro

🎉

Reduce visibility of TextMapCodec.contextFromString to package scope

3ad52db

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro reviewed Aug 17, 2018

View reviewed changes

Get rid of JaegerSpanContextTest

a1cb911

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro reviewed Aug 17, 2018

View reviewed changes

Isaac Hier added 3 commits August 17, 2018 13:54

Remove outdated comment

843c937

Signed-off-by: Isaac Hier <ihier@uber.com>

Change contextAsString to package scope

8847020

Signed-off-by: Isaac Hier <ihier@uber.com>

Style fixes

fa973ea

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro reviewed Aug 20, 2018

View reviewed changes

Move contextAsString to public method of TextMapCodec

986986e

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro reviewed Aug 20, 2018

View reviewed changes

Isaac Hier added 2 commits August 20, 2018 16:32

Specify expected string output with string formatting

48112ed

Signed-off-by: Isaac Hier <ihier@uber.com>

Use fixed IDs to avoid string formatting

900524b

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro approved these changes Aug 20, 2018

View reviewed changes

yurishkuro merged commit 27bd0e0 into jaegertracing:master Aug 20, 2018

isaachier deleted the refactor-context-from-string branch August 20, 2018 20:59

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

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Reduce visibility of TextMapCodec.contextFromString to package scope #519

isaachier commented Aug 17, 2018 •

edited

Loading

yurishkuro Aug 17, 2018

isaachier Aug 17, 2018

yurishkuro Aug 17, 2018

isaachier Aug 17, 2018

yurishkuro Aug 17, 2018 •

edited

Loading

isaachier Aug 17, 2018

yurishkuro Aug 17, 2018

codecov bot commented Aug 17, 2018 •

edited

Loading

yurishkuro Aug 17, 2018

isaachier Aug 17, 2018

yurishkuro Aug 20, 2018

yurishkuro Aug 20, 2018

isaachier Aug 20, 2018 •

edited

Loading

yurishkuro Aug 20, 2018

isaachier Aug 20, 2018

yurishkuro left a comment

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Conversation

isaachier commented Aug 17, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

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

yurishkuro Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2018 • 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

isaachier Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

isaachier commented Aug 17, 2018 •

edited

Loading

yurishkuro Aug 17, 2018 •

edited

Loading

codecov bot commented Aug 17, 2018 •

edited

Loading

isaachier Aug 20, 2018 •

edited

Loading