TraceState implementation as per spec #551

lalitb · 2021-02-01T16:07:26Z

Current tracestate api is not compliant to opentelemetry and w3c trace specs. Attempt to fix in this PR.
To Summaries the changes

As per the opentelemetry specs (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#tracestate), TraceState object is immutable, and add, update and delete methds should return newly created object. Added/updated needed methods.
Methods to serialize and de-serialize TraceState object to/from w3c trace header ( as defined here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#tracestate)
Much accurate validation check for tracestate list members using regex in non-gcc4.8 compiler.

#537

codecov · 2021-02-01T16:15:50Z

Codecov Report

Merging #551 (77e5574) into main (74ad7b1) will increase coverage by 0.08%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
+ Coverage   94.32%   94.40%   +0.08%     
==========================================
  Files         198      200       +2     
  Lines        8807     9067     +260     
==========================================
+ Hits         8307     8560     +253     
- Misses        500      507       +7

Impacted Files	Coverage Δ
api/test/trace/trace_state_test.cc	`97.79% <95.83%> (ø)`
api/include/opentelemetry/trace/trace_state.h	`97.54% <96.87%> (ø)`
sdk/test/metrics/counter_aggregator_test.cc	`98.21% <0.00%> (-1.79%)`	⬇️
sdk/src/logs/batch_log_processor.cc	`93.82% <0.00%> (-1.24%)`	⬇️
api/include/opentelemetry/nostd/string_view.h	`97.95% <0.00%> (+0.08%)`	⬆️
sdk/test/common/circular_buffer_test.cc	`100.00% <0.00%> (+1.04%)`	⬆️

pyohannes

Thanks, that looks very good! I'll give it another detailed review tomorrow.

pyohannes · 2021-02-02T01:43:00Z

api/include/opentelemetry/trace/trace_state.h


  // Returns false if no such key, otherwise returns true and populates the value parameter with the
  // associated value.
-  bool Get(nostd::string_view key, nostd::string_view &value) const noexcept
+  std::string Get(nostd::string_view key) const noexcept


I think we should preserve the ability to distinguish between an key not being specified and a key given but having an empty value.

As per w3c specs ( https://www.w3.org/TR/trace-context/#value) the value can't be empty string ( should have at-least one non-blank character ). This is validated before storing from header. But I don't have strong preference on this, we can switch back to earlier prototype if that's more clear.

Have made comments consistent with method prototype.

You're right. Your method signature is fine.

pyohannes · 2021-02-02T02:01:53Z

api/include/opentelemetry/trace/trace_state.h

+    std::regex reg_key("^[a-z0-9][a-z0-9*_\\-/]{0,255}$");
+    std::regex reg_key_multitenant(


Those can be static.

Good point. Fixed this now.

api/include/opentelemetry/trace/trace_state.h

Co-authored-by: Johannes Tax <[email protected]>

ThomsonTan · 2021-02-02T17:55:24Z

api/include/opentelemetry/trace/trace_state.h

-  // An empty TraceState.
-  TraceState() noexcept : entries_(new Entry[kMaxKeyValuePairs]), num_entries_(0) {}
+  /**
+   * Returns a newly created Tracestate parsed from the header provided.


nit: change spelling of Tracestate in this comment to TraceState?

Thanks. changed it now :)

api/include/opentelemetry/trace/trace_state.h

api/test/trace/trace_state_test.cc

pyohannes · 2021-02-03T00:51:41Z

api/test/trace/trace_state_test.cc

+  auto ts2_new       = ts2.Set("n_k1", "n_v1");  // adding to max list, should fail and return empty
+  EXPECT_EQ(ts2_new.ToHeader(), "");


What consequence would this behavior have for the user?

Let's say one receives a tracestate header with 32 entries, then adds one (this returns an empty tracestate), and then passes on the header. If the user doesn't do explicit checks, an empty header will be passed on. I don't think that's ideal.

What do you think about Set just returning the unaltered tracestate when the limit is exceeded?

Yes, I didn't find anything in specs on how to address this, and different languages implementation are behaving differently ( python returns empty, JS doesn't do any check on limit, java returns reference to itself ). Said that, it make sense to return unaltered tracestate instead of empty. I will do the change. Thanks for the comment.

This is fixed as discussed.

Co-authored-by: Johannes Tax <[email protected]>

ThomsonTan · 2021-02-03T04:03:12Z

api/include/opentelemetry/trace/trace_state.h

+      else
+      {
+        // `end` points to end of current list member
+        end = end - 1;


Just --end? Also could it get -1 if the first character is ,?

Made it --end. If would be fine if first char is ,, as this will make control to go to below condition:

if (list_member.size() == 0) { // empty list member, move to next in list begin = end + 2; // begin points to start of next member continue; }

and process next list member. I have added test case to validate this.

If the first char is ,, then end will be -1, so the call to TrimString will trigger access violation when accessing the input string, like header[static_cast<size_t>(-1)] which happens before the empty list_member check?

Yes, you are right and bazel asan fails because of that. I am fixing it now :) Thanks for pointing out the issue.

Ok, the easy solution was to add special conditional check for , as first char.

ThomsonTan · 2021-02-03T04:22:32Z

api/include/opentelemetry/trace/trace_state.h

+  /**
+   * Returns `new` TransState object after removing the attribute with given key ( if present )
+   * @returns empty TransState object if key is invalid
+   * @returns copy of original TransState object if key is not present (??)


Suggested change

* @returns copy of original TransState object if key is not present (??)

* @returns copy of original TraceState object if key is not present (??)

Seems for now an empty instance is returned if the key is not present, should we return original TraceState like mentioned in the comment?

Thanks for suggestion TransState. Have changed it now as part of other changes.
Also,
empty instance is returned if the key is not present,

As of now, as per code it return empty instance if key is not valid, and copy of original instance if key is not present.

ThomsonTan · 2021-02-03T04:26:58Z

api/include/opentelemetry/trace/trace_state.h

+
+  static nostd::string_view TrimString(nostd::string_view str, size_t left, size_t right)
+  {
+    while (str[(std::size_t)right] == ' ' && left < right)


nit: C++ style static_cast<std::size_t>?

Thanks. fixed it now.

ThomsonTan · 2021-02-03T07:16:55Z

api/include/opentelemetry/trace/trace_state.h

+
+      auto entry = (entries_.get())[count];
+      header_s.append(std::string(entry.GetKey()));
+      header_s.append(std::string(1, kKeyValueSeparator));


No need to construct a temporary std::string here, just pass kKeyValueSeparator to std::string::append?

thanks fixed.

lalitb · 2021-02-04T04:59:14Z

@ThomsonTan - Thanks for your review. Can we merge this PR if there are no further comments. I have a another PR dependent on these changes so the ask :)

lalitb added 8 commits February 1, 2021 17:41

fix trace state as per specs

925c6a3

fix format

7f4ecfa

fix

8f7ba83

fix gcc4.8

0751130

fix

24b95b7

fix

c067b23

fix

ba4ded2

fix

70a2a26

lalitb requested a review from a team February 1, 2021 16:07

lalitb added the area:api label Feb 1, 2021

fix

34e9759

lalitb changed the title ~~TraceState api implementation as per spec~~ TraceState implementation as per spec Feb 1, 2021

pyohannes reviewed Feb 2, 2021

View reviewed changes

lalitb and others added 3 commits February 2, 2021 21:58

Update api/include/opentelemetry/trace/trace_state.h

e6c1204

Co-authored-by: Johannes Tax <[email protected]>

review comments

2b1be89

Merge branch 'main' into tracestate-fix

0d23d00

ThomsonTan reviewed Feb 2, 2021

View reviewed changes

pyohannes reviewed Feb 3, 2021

View reviewed changes

lalitb and others added 4 commits February 3, 2021 09:14

Update api/include/opentelemetry/trace/trace_state.h

1a54cdc

Co-authored-by: Johannes Tax <[email protected]>

Update api/include/opentelemetry/trace/trace_state.h

47977cd

Co-authored-by: Johannes Tax <[email protected]>

Update api/include/opentelemetry/trace/trace_state.h

f3da416

Co-authored-by: Johannes Tax <[email protected]>

Update api/test/trace/trace_state_test.cc

cc41067

Co-authored-by: Johannes Tax <[email protected]>

ThomsonTan reviewed Feb 3, 2021

View reviewed changes

lalitb added 4 commits February 3, 2021 11:35

review fix

1c27314

review fix

228f2a7

fix

9b60b48

fix

ce24183

lalitb added 2 commits February 3, 2021 11:58

fix

65b81e5

fix

a03fe1b

ThomsonTan reviewed Feb 3, 2021

View reviewed changes

lalitb added 2 commits February 3, 2021 13:02

fix

7413c85

Merge branch 'main' into tracestate-fix

6acf4d6

pyohannes approved these changes Feb 4, 2021

View reviewed changes

ThomsonTan approved these changes Feb 4, 2021

View reviewed changes

lalitb merged commit ea87130 into open-telemetry:main Feb 4, 2021

This was referenced Feb 5, 2021

TraceState interface is not compliant with specs. #537

Closed

Integrate TraceState with http_trace_context propagator #563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TraceState implementation as per spec #551

TraceState implementation as per spec #551

lalitb commented Feb 1, 2021 •

edited

Loading

codecov bot commented Feb 1, 2021 •

edited

Loading

pyohannes left a comment

pyohannes Feb 2, 2021

lalitb Feb 2, 2021 •

edited

Loading

lalitb Feb 2, 2021

pyohannes Feb 3, 2021

pyohannes Feb 2, 2021

lalitb Feb 2, 2021

ThomsonTan Feb 2, 2021

lalitb Feb 3, 2021

pyohannes Feb 3, 2021

lalitb Feb 3, 2021

lalitb Feb 3, 2021

ThomsonTan Feb 3, 2021

lalitb Feb 3, 2021

ThomsonTan Feb 3, 2021

lalitb Feb 3, 2021 •

edited

Loading

lalitb Feb 3, 2021

ThomsonTan Feb 3, 2021

lalitb Feb 3, 2021 •

edited

Loading

ThomsonTan Feb 3, 2021

lalitb Feb 3, 2021

ThomsonTan Feb 3, 2021

lalitb Feb 3, 2021

lalitb commented Feb 4, 2021

		std::regex reg_key("^[a-z0-9][a-z0-9*_\\-/]{0,255}$");
		std::regex reg_key_multitenant(

		auto ts2_new = ts2.Set("n_k1", "n_v1"); // adding to max list, should fail and return empty
		EXPECT_EQ(ts2_new.ToHeader(), "");

	* @returns copy of original TransState object if key is not present (??)
	* @returns copy of original TraceState object if key is not present (??)

TraceState implementation as per spec #551

TraceState implementation as per spec #551

Conversation

lalitb commented Feb 1, 2021 • edited Loading

codecov bot commented Feb 1, 2021 • edited Loading

Codecov Report

pyohannes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Feb 2, 2021 • 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

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

lalitb Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Feb 3, 2021 • 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

lalitb commented Feb 4, 2021

lalitb commented Feb 1, 2021 •

edited

Loading

codecov bot commented Feb 1, 2021 •

edited

Loading

lalitb Feb 2, 2021 •

edited

Loading

lalitb Feb 3, 2021 •

edited

Loading

lalitb Feb 3, 2021 •

edited

Loading