Update the TraceState key validations to more closely match the w3c spec by jkwatson · Pull Request #1561 · open-telemetry/opentelemetry-java

jkwatson · 2020-08-19T18:30:42Z

Resolves #1027

…pec.

codecov · 2020-08-19T18:43:01Z

Codecov Report

Merging #1561 into master will increase coverage by 0.06%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #1561      +/-   ##
============================================
+ Coverage     86.71%   86.78%   +0.06%     
- Complexity     1394     1401       +7     
============================================
  Files           162      162              
  Lines          5353     5365      +12     
  Branches        513      519       +6     
============================================
+ Hits           4642     4656      +14     
+ Misses          525      524       -1     
+ Partials        186      185       -1

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/io/opentelemetry/trace/TraceState.java	`89.18% <88.88%> (+2.09%)`	`37.00 <23.00> (+7.00)`
...try/sdk/metrics/aggregator/LongMinMaxSumCount.java	`100.00% <0.00%> (+3.92%)`	`6.00% <0.00%> (ø%)`

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 3a8ed95...585922d. Read the comment docs.

anuraaga · 2020-08-20T06:12:53Z

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

      }
-      if ((c == '@') && (++atSeenCount > 1)) {
-        return false;
+      int atPosition = 0;


I'm having trouble grokking this code.

Sorry accidentally hit Add single comment, continuing below

anuraaga · 2020-08-20T06:14:31Z

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

+        atSeenCount++;
+        if (atSeenCount > 1) {
+          return false;
+        }


Instead of having atPosition is it ok to do something like this in this branch?

// remaining is vendor id, must be 13 characters or less if (key.length() - i > 13) { return false; }

anuraaga · 2020-08-20T06:15:15Z

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

      }
+      if (atSeenCount == 1) {
+        // vendor id must be 13 characters or less
+        if ((i - atPosition) > 13) {


I may just be misgrokking, but isn't atPosition 0 for most of the loop rather than the actual atPosition? If my reading is right, my above suggestion would probably solve it but it also means there's a unit test missing for the current buggy state.

atSeenCount is incremented only when atPosition changes to i. This way, this if is checked only for atPosition > 0.

Yeah but atPosition is declared inside the loop, which I think is the original bug. So except for when key[i] == '@', atPosition == 0 for the remainder of the loop since it's initialized to 0. Sorry if I'm missing it though I've tried to stare long and this is what I came up with.

Oh! You are totally right! I miss that bit! We are indeed missing a test here!

I'm not following the problem. Can you give me a key that we're not handling correctly, so I can add a unit test for the issue?

Note, I'm rewriting this chunk of code to make it clearer. But, having an example key that we're not handling correctly would be very useful (because I don't see the issue).

I've pushed the rewrite. Let me know if that helps at all with understanding what's going on in here.

And, please give me a concrete key example case we're not handling correctly if there is one. :)

I'm pretty sure 12345678901234567890@nr is valid but fails when I added this to your branch. Basically any time the tenant id is longer than 13 characters, since the current logic is resetting the atPosition to the beinning of the string.

My suggestion of not bothering with atPosition still stands :)

@Test void testValidLongVendorKey() { TraceState result = EMPTY.toBuilder().set("12345678901234567890@nr", FIRST_VALUE).build(); assertThat(result.get("12345678901234567890@nr")).isEqualTo(FIRST_VALUE); }

doh! I know you said it like 5 times, but I totally missed that we were resetting the atPosition inside the loop. 🤦

fixed and pushed.

anuraaga · 2020-08-20T06:17:33Z

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

+    }
+    if (atSeenCount == 0) {
+      // if it's not the vendor format (with an '@' sign), the key must start with a letter.
+      return !Character.isDigit(key.charAt(0));


Also, guess we should avoid unicode comparisons like this and stick with simple ones we implement

that's fair. I'll extract the digit logic into a separate method and us that.

anuraaga

Thanks!

Oberon00 · 2020-08-24T10:23:21Z

I think we should revert this change. We should follow the "be liberal in what you accept" guideline here, otherwise any deployed opentelemetry-java SDK will break tracestate entries that make use of the relaxed grammar in the current draft spec (once it graduates from being a draft).

jkwatson · 2020-08-24T14:50:32Z

I think we should revert this change. We should follow the "be liberal in what you accept" guideline here, otherwise any deployed opentelemetry-java SDK will break tracestate entries that make use of the relaxed grammar in the current draft spec (once it graduates from being a draft).

If we want to truly "be liberal in what we accept", we should drop all validations of the trace state entries and just pass them through un-molested. See this issue: #876

Update the TraceState key validations to more closely match the w3c s…

d4ac2ea

…pec.

jkwatson requested review from anuraaga, arminru, bogdandrutu, carlosalberto, pavolloffay, thisthat and tylerbenson as code owners August 19, 2020 18:30

carlosalberto approved these changes Aug 19, 2020

View reviewed changes

thisthat approved these changes Aug 20, 2020

View reviewed changes

anuraaga reviewed Aug 20, 2020

View reviewed changes

jwatson added 2 commits August 20, 2020 11:32

rework method and variable names for more clarity

18f2031

Add some tests and fix a bug

585922d

anuraaga approved these changes Aug 23, 2020

View reviewed changes

jkwatson merged commit 79b6f0e into open-telemetry:master Aug 23, 2020

Conversation

jkwatson commented Aug 19, 2020

Uh oh!

codecov bot commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Oberon00 commented Aug 24, 2020

Uh oh!

jkwatson commented Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Aug 19, 2020 •

edited

Loading