SpanContext hides the TraceId/SpanId implementations #1594

jkwatson · 2020-08-26T21:33:26Z

This removes the usage of wrapper classes for the ids.
Changes the SpanContext creation methods to accept CharSequences and an offset for the ids.
Changes the IdGenerator interface to generate String ids.

Relevant issue: #1314
Extensive discussion about this took place in this draft PR: #1374

jkwatson · 2020-08-26T21:38:59Z

I apologize for the size of this PR. Most of it is mechanical changes in types, so hopefully review will go quickly.

codecov · 2020-08-26T21:50:53Z

Codecov Report

Merging #1594 into master will decrease coverage by 0.03%.
The diff coverage is 93.61%.

@@             Coverage Diff              @@
##             master    #1594      +/-   ##
============================================
- Coverage     91.16%   91.12%   -0.04%     
- Complexity      991      995       +4     
============================================
  Files           117      117              
  Lines          3668     3664       -4     
  Branches        340      345       +5     
============================================
- Hits           3344     3339       -5     
- Misses          218      219       +1     
  Partials        106      106

Impacted Files	Coverage Δ	Complexity Δ
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	`81.89% <0.00%> (ø)`	`78.00 <0.00> (ø)`
.../main/java/io/opentelemetry/sdk/trace/Sampler.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...java/io/opentelemetry/sdk/trace/data/SpanData.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ions/trace/jaeger/sampler/JaegerRemoteSampler.java	`68.62% <ø> (ø)`	`9.00 <0.00> (ø)`
...ions/trace/jaeger/sampler/PerOperationSampler.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ions/trace/jaeger/sampler/RateLimitingSampler.java	`70.83% <ø> (ø)`	`7.00 <0.00> (ø)`
.../main/java/io/opentelemetry/trace/SpanContext.java	`87.50% <75.00%> (-12.50%)`	`11.00 <8.00> (+3.00)`	⬇️
...java/io/opentelemetry/trace/BigendianEncoding.java	`94.28% <93.33%> (-0.26%)`	`31.00 <13.00> (+13.00)`	⬇️
.../src/main/java/io/opentelemetry/trace/TraceId.java	`96.29% <95.65%> (+1.05%)`	`14.00 <12.00> (-9.00)`	⬆️
...i/src/main/java/io/opentelemetry/trace/SpanId.java	`94.11% <100.00%> (-5.89%)`	`12.00 <12.00> (-8.00)`
... and 8 more

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 225acce...85aafd2. Read the comment docs.

anuraaga

Thanks!

api/src/main/java/io/opentelemetry/trace/BigendianEncoding.java

api/src/main/java/io/opentelemetry/trace/SpanContext.java

...ns/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/Common.java

api/src/main/java/io/opentelemetry/trace/SpanContext.java

api/src/main/java/io/opentelemetry/trace/SpanId.java

api/src/main/java/io/opentelemetry/trace/TraceId.java

...ain/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorInjectorSingleHeader.java

api/src/main/java/io/opentelemetry/trace/propagation/HttpTraceContext.java

jkwatson · 2020-08-27T18:28:10Z

@anuraaga I think I got all of your comments addressed. Thanks so much for taking the time to give it a thorough review!

anuraaga

Just nits, thanks!

api/src/main/java/io/opentelemetry/trace/BigendianEncoding.java

api/src/main/java/io/opentelemetry/trace/SpanId.java

anuraaga · 2020-08-28T06:31:33Z

api/src/main/java/io/opentelemetry/trace/BigendianEncoding.java

+    for (int i = 0; i < value.length(); i++) {
+      char b = value.charAt(i);
+      // 48..57 && 97..102 are valid
+      if ((b < 48 || b > 57) && (b < 97 || b > 102)) {


I'm pretty sure they're the same, and maybe it's just me but I find writing the same code as the comment would be more readable

Suggested change

if ((b < 48 || b > 57) && (b < 97 || b > 102)) {

if (!(b >= 48 && b <= 57) || !(b >= 97 && b <= 102)) {

heh. I agree with you. However, what you pasted isn't correct (IDEA says it's always true!). I'll figure out how to make it what you intended. :)

check out the update. I think it's now much easier to understand.

api/src/test/java/io/opentelemetry/trace/TraceIdTest.java

.../java/io/opentelemetry/extensions/trace/propagation/B3PropagatorInjectorMultipleHeaders.java

...ropagators/src/main/java/io/opentelemetry/extensions/trace/propagation/JaegerPropagator.java

...pagators/src/main/java/io/opentelemetry/extensions/trace/propagation/OtTracerPropagator.java

opentracing_shim/src/test/java/io/opentelemetry/opentracingshim/testbed/TestUtils.java

jkwatson · 2020-08-28T18:34:31Z

ok, all nits have been de-nitted!

anuraaga

LGTM thanks

jkwatson · 2020-08-29T01:43:29Z

I'm going to hold off a few days to merge this, both to delay to after the 0.8.0 release, and to give others (@carlosalberto and @bogdandrutu) to chime in with any issue they might have, as this is a significant API change.

tylerbenson · 2020-09-01T21:02:53Z

api/src/main/java/io/opentelemetry/trace/SpanContext.java

+  abstract String getTraceId();
+
+  abstract String getSpanId();


return CharSequence instead?

I think we discussed this, and decided to keep it as String for now. We can always open it up to CharSequence later, since it's a broader interface, I think.

You can change return types that easily. If clients expect String they will break on CharSequence. You can widen input, not output

true, but this isn't a public method!

bogdandrutu · 2020-09-01T21:09:40Z

api/src/main/java/io/opentelemetry/trace/SpanId.java

   */
-  public static SpanId getInvalid() {
-    return INVALID;
+  public static int getBase16Length() {


Hex? If you used hex in SpanContext method names.

yes! should fix this. good catch.

bogdandrutu · 2020-09-01T21:11:36Z

api/src/main/java/io/opentelemetry/trace/SpanId.java

+    return new String(result);
+  }
+
+  private static char[] getBuffer() {


s/getBuffer/getTemporaryBuffer?

bogdandrutu · 2020-09-01T21:11:57Z

api/src/main/java/io/opentelemetry/trace/SpanId.java

-   */
-  public void copyLowerBase16To(char[] dest, int destOffset) {
-    BigendianEncoding.longToBase16String(id, dest, destOffset);
+  public static byte[] bytesFromLowerBase16(String src, int srcOffset) {


Same, use Hex if you decided to use that in the SC

agreed. 👍

bogdandrutu · 2020-09-01T21:13:04Z

api/src/main/java/io/opentelemetry/trace/SpanContext.java

@@ -59,8 +60,13 @@ public static SpanContext getInvalid() {
   * @since 0.1.0
   */
  public static SpanContext create(
-      TraceId traceId, SpanId spanId, TraceFlags traceFlags, TraceState traceState) {
-    return new AutoValue_SpanContext(traceId, spanId, traceFlags, traceState, /* remote=*/ false);
+      String traceId, String spanId, TraceFlags traceFlags, TraceState traceState) {


name the params traceIdHex, spanIdHex

� This is the 1st commit message: WIP on converting to String-based SpanContext don't hand out the byte arrays publicly, but require making copies make sure to hand out fresh invalid byte arrays. Use strings for span and trace ids. Switch over to CharSequence instead of String for the ids Fix a couple of places that were casting to String Add some simple wrappers for the generated longs to save converting until the last moment to the character-based representation. introduce a reusable threadlocal char buffer for generating random ids. update for changes from upstream Change the SpanContext to store the ids as Strings internally Change the id access methods on SpanContext to be clearly labeled as the base16 representations Add a new create method that allows specifying offsets for traceId and spanId CharSequences Provide an option for creating a SpanContext from longs or Strings, optionally. fix a typo update from upstream � The commit message #2 will be skipped: � don't hand out the byte arrays publicly, but require making copies

… Span ids.

Co-authored-by: Anuraag Agrawal <[email protected]>

…tensions/trace/propagation/B3PropagatorInjectorSingleHeader.java Co-authored-by: Anuraag Agrawal <[email protected]>

Co-authored-by: Anuraag Agrawal <[email protected]>

carlosalberto · 2020-09-03T13:43:45Z

Amazing job @jkwatson sorry for the late review!

anuraaga · 2020-09-03T23:44:16Z

Awesome!

Having abstract methods with default visibility makes the class not able to be subclassed by other packages. Changing to protected to maintain the intended restrictions from open-telemetry#1594, but allowing it to be subclassed.

jkwatson requested review from anuraaga, arminru, bogdandrutu, carlosalberto, pavolloffay, thisthat and tylerbenson as code owners August 26, 2020 21:33

jkwatson force-pushed the string_ids branch from c0e2b21 to 90dc14a Compare August 26, 2020 21:36

jkwatson mentioned this pull request Aug 26, 2020

Get rid of TraceId/SpanId wrappers, and have the SpanContext hold the ids #1374

Closed

anuraaga reviewed Aug 27, 2020

View reviewed changes

anuraaga approved these changes Aug 28, 2020

View reviewed changes

anuraaga approved these changes Aug 29, 2020

View reviewed changes

jkwatson added the API API related issues label Aug 29, 2020

tylerbenson reviewed Sep 1, 2020

View reviewed changes

bogdandrutu reviewed Sep 1, 2020

View reviewed changes

jwatson and others added 10 commits September 1, 2020 14:23

WIP on converting to String-based SpanContext

2053731

Update the SpanContext to only store Strings internally for Trace and…

45347b6

… Span ids.

remove dead files

7b279dd

cleanup some CharSequence usages to String

ad89249

Update api/src/main/java/io/opentelemetry/trace/BigendianEncoding.java

8135b10

Co-authored-by: Anuraag Agrawal <[email protected]>

Update extensions/trace_propagators/src/main/java/io/opentelemetry/ex…

f1000fa

…tensions/trace/propagation/B3PropagatorInjectorSingleHeader.java Co-authored-by: Anuraag Agrawal <[email protected]>

Update api/src/main/java/io/opentelemetry/trace/SpanId.java

72783e2

Co-authored-by: Anuraag Agrawal <[email protected]>

Update api/src/main/java/io/opentelemetry/trace/TraceId.java

6522ed3

Co-authored-by: Anuraag Agrawal <[email protected]>

updates from PR suggestion fubars

d0f0b12

jkwatson and others added 5 commits September 1, 2020 14:23

some cleanup from PR feedback

410ce78

Switch back to String from CharSequence

362cba9

Get rid of the overload with the offsets

07a9eec

Lots of bits of cleanup from PR feedback.

4bf2056

more renaming of base16 to hex

85aafd2

jkwatson force-pushed the string_ids branch from a51046e to 85aafd2 Compare September 1, 2020 21:43

carlosalberto approved these changes Sep 3, 2020

View reviewed changes

jkwatson merged commit 485cc52 into open-telemetry:master Sep 3, 2020

jkwatson deleted the string_ids branch September 3, 2020 17:03

tylerbenson mentioned this pull request Oct 30, 2020

Allow subclassing SpanContext by other packages #1940

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpanContext hides the TraceId/SpanId implementations #1594

SpanContext hides the TraceId/SpanId implementations #1594

jkwatson commented Aug 26, 2020 •

edited

Loading

jkwatson commented Aug 26, 2020

codecov bot commented Aug 26, 2020 •

edited

Loading

anuraaga left a comment

jkwatson commented Aug 27, 2020

anuraaga left a comment

anuraaga Aug 28, 2020

jkwatson Aug 28, 2020 •

edited

Loading

jkwatson Aug 28, 2020

jkwatson commented Aug 28, 2020

anuraaga left a comment

jkwatson commented Aug 29, 2020

tylerbenson Sep 1, 2020

jkwatson Sep 1, 2020

iNikem Sep 2, 2020

jkwatson Sep 2, 2020

bogdandrutu Sep 1, 2020

jkwatson Sep 1, 2020

jkwatson Sep 1, 2020 •

edited

Loading

bogdandrutu Sep 1, 2020

jkwatson Sep 1, 2020

bogdandrutu Sep 1, 2020

jkwatson Sep 1, 2020

jkwatson Sep 1, 2020

bogdandrutu Sep 1, 2020

jkwatson Sep 1, 2020

carlosalberto commented Sep 3, 2020

anuraaga commented Sep 3, 2020

	if ((b < 48 \|\| b > 57) && (b < 97 \|\| b > 102)) {
	if (!(b >= 48 && b <= 57) \|\| !(b >= 97 && b <= 102)) {

SpanContext hides the TraceId/SpanId implementations #1594

SpanContext hides the TraceId/SpanId implementations #1594

Conversation

jkwatson commented Aug 26, 2020 • edited Loading

jkwatson commented Aug 26, 2020

codecov bot commented Aug 26, 2020 • edited Loading

Codecov Report

anuraaga left a comment

Choose a reason for hiding this comment

jkwatson commented Aug 27, 2020

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Aug 28, 2020

anuraaga left a comment

Choose a reason for hiding this comment

jkwatson commented Aug 29, 2020

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

jkwatson Sep 1, 2020 • 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

carlosalberto commented Sep 3, 2020

anuraaga commented Sep 3, 2020

jkwatson commented Aug 26, 2020 •

edited

Loading

codecov bot commented Aug 26, 2020 •

edited

Loading

jkwatson Aug 28, 2020 •

edited

Loading

jkwatson Sep 1, 2020 •

edited

Loading