Adding B3 propagator #523

TomRoSystems · 2021-01-19T23:37:36Z

Basing on https://github.com/open-telemetry/opentelemetry-cpp/blob/880e392e10b6d8e964ade2e22d7231950d6d2443/api/include/opentelemetry/trace/propagation/http_trace_context.h

codecov · 2021-01-19T23:47:30Z

Codecov Report

Merging #523 (f52ce7b) into main (d94a368) will increase coverage by 0.12%.
The diff coverage is 99.13%.

@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
+ Coverage   94.17%   94.30%   +0.12%     
==========================================
  Files         195      197       +2     
  Lines        8572     8802     +230     
==========================================
+ Hits         8073     8301     +228     
- Misses        499      501       +2

Impacted Files	Coverage Δ
...de/opentelemetry/trace/propagation/b3_propagator.h	`97.95% <97.95%> (ø)`
api/include/opentelemetry/nostd/string_view.h	`97.87% <100.00%> (+0.37%)`	⬆️
api/test/nostd/string_view_test.cc	`100.00% <100.00%> (ø)`
api/test/trace/propagation/b3_propagation_test.cc	`100.00% <100.00%> (ø)`
sdk/test/metrics/counter_aggregator_test.cc	`98.21% <0.00%> (-1.79%)`	⬇️
sdk/test/common/circular_buffer_test.cc	`100.00% <0.00%> (+1.02%)`	⬆️

lalitb

Thanks for working on this. Looks good to me in general. Would wait for other's feedback for proper place for it:

repo_root/api/include/opentelemetry/trace/propagation/ or
repo_root/propagators

As per Opentelemetry specs (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#propagators-distribution ) - only W3C Tracecontext and W3C Baggage MAY be distributed as part of API package, and rest as extension packages.

api/include/opentelemetry/trace/propagation/b3_propagator.h

lalitb · 2021-01-20T14:13:32Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+  }
+};
+
+// The B3Propagator class provides an interface that enables extracting and injecting


Would header description as below more readable ?

// B3Propogator class provides interface that enables extracting and injecting context into single header of HTTP Request.
// B3PropogatorMultiHeader class provides interface that enables extracting and injecting context into multiple headers of HTTP Request.

lalitb · 2021-01-20T15:50:51Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+  {
+    uint8_t buf[kTraceIdHexStrLength / 2];
+    uint8_t *b_ptr = buf;
+    GenerateHexFromString(trace_id, kTraceIdHexStrLength, b_ptr);


As B3 trace_id could also be 16 hex chars, while we are always trying to read 32 chars/bytes here. This may result in reading past the hex string ?

Yes! Thanks for pointing this out. I'll create fix and test for such case.

api/include/opentelemetry/trace/propagation/b3_propagator.h

TomRoSystems · 2021-01-22T14:03:20Z

Thanks for working on this. Looks good to me in general. Would wait for other's feedback for proper place for it:

repo_root/api/include/opentelemetry/trace/propagation/ or
repo_root/propagators

As per Opentelemetry specs (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#propagators-distribution ) - only W3C Tracecontext and W3C Baggage MAY be distributed as part of API package, and rest as extension packages.

Thank you for review.

Any more opinions on this one? In go there's propagation directory same in dotnet whereas Java has extensions directory on top level.

- added support for TraceId 16 and 32 length - added tests for scenario above and inject multi-header nostd::string_view - added find for single character

seemk · 2021-01-25T12:01:38Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+
+  // Converts a single character to a corresponding integer (e.g. '1' to 1), return -1
+  // if the character is not a valid number in hex.
+  static uint8_t HexToInt(char c)


Perhaps we should move hex to int conversion to a shared part? Currently the method (+ comment) is duplicated by b3 and http propagators.

seemk · 2021-01-25T12:03:20Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+      // From: https://github.com/openzipkin/b3-propagation/blob/master/RATIONALE.md
+      // trace_id can be 16 or 32 chars
+      auto firstSep = singleB3Header.find('-');
+      trace_id      = singleB3Header.substr(0, firstSep);


This will allow an invalid length trace id and span id to pass through, e.g. a-b will produce 000000000000000a as the trace ID.

seemk · 2021-01-25T12:17:09Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+        {
+          hexDigit1 = HexToInt(hexStr[--posInp]);
+        }
+        if (hexDigit1 < 0 || hexDigit2 < 0)


This branch is never taken as HexToInt returns uint8_t (uint8_t(-1) = int(255)), thus a malformed hex sequence is generated.

Yep :( copied from here...

opentelemetry-cpp/api/include/opentelemetry/trace/propagation/http_trace_context.h

Lines 120 to 123 in ce89aa1

int tmp1 = HexToInt(trace_flags[0]);

int tmp2 = HexToInt(trace_flags[1]);

if (tmp1 < 0 || tmp2 < 0)

return TraceFlags(0); // check for invalid char

seemk · 2021-01-26T08:33:27Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+    }
+  }
+
+  static SpanContext ExtractImpl(Getter getter, const T &carrier)


Might be a nitpick, but I think we should validate the parsed trace_id, span_id lengths for validity. Say if a malformed header with too large of a trace_id gets sent, currently this logic will produce a valid SpanContext, but with some of the trace_id cut off - my opinion is we should just fail early in these cases.

Thanks.
Would like to get more opinion on this topic Robustness vs it's potential harmful consequences from others as I have not found clear guidance on this one.

In case of any error in parsing trace_id and span_id, the right approach should be to set them invalid ( i.e. all zeros ) and return the SpanContext. This will ensure that SpanContext is also invalid.

maxgolov · 2021-01-27T19:00:12Z

api/include/opentelemetry/nostd/string_view.h

@@ -120,6 +120,20 @@ class string_view
    return substr(pos1, count1).compare(string_view(s, count2));
  };

+  size_type find(char ch, size_type pos = 0) const noexcept


Looks good. Minor concern is - since we're adding it here, it would'be been ideal to add all overloads:

constexpr size_type find(basic_string_view v, size_type pos = 0) const noexcept; constexpr size_type find(CharT ch, size_type pos = 0) const noexcept; constexpr size_type find(const CharT* s, size_type pos, size_type count) const; constexpr size_type find(const CharT* s, size_type pos = 0) const;

I'd like to avoid Scope creep and complaints related to complexity for those operations.

pyohannes · 2021-01-28T01:02:38Z

Sorry, I'm a bit late with my comment.

As per Opentelemetry specs - only W3C Tracecontext and W3C Baggage MAY be distributed as part of API package, and rest as extension packages.

Would it be better to put this under ext, instead of api? This would be more true to the spec, and help to keep the API part small. Java has those in an extension directory.

ThomsonTan · 2021-01-28T06:16:14Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+
+// The B3PropagatorExtractor class provides an interface that enables extracting context from
+// headers of HTTP requests. HTTP frameworks and clients can integrate with B3Propagator by
+// providing the object  containing the headers, and a getter function for the extraction.


nit: extra space after object.

ThomsonTan · 2021-01-28T06:29:11Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+  static TraceId GenerateTraceIdFromString(nostd::string_view trace_id)
+  {
+    uint8_t buf[kTraceIdHexStrLength / 2];
+    uint8_t *b_ptr = buf;


b_ptr is unused now?

ThomsonTan · 2021-01-28T06:29:34Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+  static SpanId GenerateSpanIdFromString(nostd::string_view span_id)
+  {
+    uint8_t buf[kSpanIdHexStrLength / 2];
+    uint8_t *b_ptr = buf;


ThomsonTan · 2021-01-28T06:34:03Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+  {
+    if (c >= '0' && c <= '9')
+    {
+      return (int)(c - '0');


Convert return value to int8_t to be consistent with the declared return type?

ThomsonTan · 2021-01-28T06:36:52Z

api/include/opentelemetry/trace/propagation/b3_propagator.h

+    {
+      return;
+    }
+    char trace_id[32];


Declare the length with the constants defined in the begining of this file?

TomRoSystems · 2021-01-28T14:48:19Z

@lalitb @ThomsonTan
Guys I've created #539 and will move rest of work over there.

Can this be merged now?

lalitb · 2021-01-28T14:55:17Z

@lalitb @ThomsonTan
Guys I've created #539 and will move rest of work over there.

Can this be merged now?

Thanks @TomRoSystems . @pyohannes suggested on moving B3 propagator to ext directory. If you agree on that, we can either do it as part of #539, or have a separate issue to let someone pick it up.

TomRoSystems · 2021-01-28T14:59:24Z

@lalitb @ThomsonTan
Guys I've created #539 and will move rest of work over there.
Can this be merged now?

Thanks @TomRoSystems . @pyohannes suggested on moving B3 propagator to ext directory. If you agree on that, we can either do it as part of #539, or have a separate issue to let someone pick it up.

I'd like to do it as part of #539

Adding B3 propagator

415bd5b

TomRoSystems requested a review from a team January 19, 2021 23:37

lalitb reviewed Jan 20, 2021

View reviewed changes

api/include/opentelemetry/trace/propagation/b3_propagator.h Outdated Show resolved Hide resolved

lalitb reviewed Jan 20, 2021

View reviewed changes

api/include/opentelemetry/trace/propagation/b3_propagator.h Show resolved Hide resolved

TomRoSystems added 5 commits January 22, 2021 20:18

B3 Propoagation:

2aba3f9

- added support for TraceId 16 and 32 length - added tests for scenario above and inject multi-header nostd::string_view - added find for single character

Fix formatting

6c94b36

Make traceId left-padded

37269ee

Merge branch 'master' into b3_propagator

11ac503

Rename SpanContext.HasRemoteParent to SpanContext.IsRemote

8c78d51

TomRoSystems requested review from lalitb and bogdandrutu January 22, 2021 21:37

lalitb approved these changes Jan 25, 2021

View reviewed changes

seemk suggested changes Jan 25, 2021

View reviewed changes

lalitb requested a review from pyohannes January 25, 2021 19:35

TomRoSystems added 2 commits January 25, 2021 20:36

Fix for condition always false + test for invalid hex value

bb74736

Proper fix with latest master branch

643602b

Base automatically changed from master to main January 26, 2021 00:46

seemk reviewed Jan 26, 2021

View reviewed changes

maxgolov reviewed Jan 27, 2021

View reviewed changes

ThomsonTan reviewed Jan 28, 2021

View reviewed changes

ThomsonTan approved these changes Jan 28, 2021

View reviewed changes

Remove unused variables. Use constants for array sizes.

1a8bb3c

TomRoSystems mentioned this pull request Jan 28, 2021

Fix condition always false in http_trace_context and create common utils file for propagators #539

Closed

Merge branch 'main' into b3_propagator

f52ce7b

lalitb merged commit eccafd5 into open-telemetry:main Jan 28, 2021

TomRoSystems mentioned this pull request Jan 29, 2021

Adding B3 propagator TomRoSystems/opentelemetry-cpp#1

Closed

TomRoSystems mentioned this pull request Feb 11, 2021

REQUEST: New membership for TomRoSystems open-telemetry/community#647

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding B3 propagator #523

Adding B3 propagator #523

TomRoSystems commented Jan 19, 2021

codecov bot commented Jan 19, 2021 •

edited

Loading

lalitb left a comment •

edited

Loading

lalitb Jan 20, 2021 •

edited

Loading

lalitb Jan 20, 2021 •

edited

Loading

TomRoSystems Jan 22, 2021

TomRoSystems commented Jan 22, 2021

seemk Jan 25, 2021

seemk Jan 25, 2021

seemk Jan 25, 2021

TomRoSystems Jan 25, 2021

seemk Jan 26, 2021

TomRoSystems Jan 26, 2021

lalitb Jan 26, 2021 •

edited

Loading

maxgolov Jan 27, 2021

TomRoSystems Jan 27, 2021 •

edited

Loading

pyohannes commented Jan 28, 2021

ThomsonTan Jan 28, 2021

ThomsonTan Jan 28, 2021

ThomsonTan Jan 28, 2021

ThomsonTan Jan 28, 2021

ThomsonTan Jan 28, 2021

TomRoSystems commented Jan 28, 2021

lalitb commented Jan 28, 2021

TomRoSystems commented Jan 28, 2021

	int tmp1 = HexToInt(trace_flags[0]);
	int tmp2 = HexToInt(trace_flags[1]);
	if (tmp1 < 0 \|\| tmp2 < 0)
	return TraceFlags(0); // check for invalid char

Adding B3 propagator #523

Adding B3 propagator #523

Conversation

TomRoSystems commented Jan 19, 2021

codecov bot commented Jan 19, 2021 • edited Loading

Codecov Report

lalitb left a comment • edited Loading

Choose a reason for hiding this comment

lalitb Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomRoSystems commented Jan 22, 2021

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 Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomRoSystems Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

pyohannes commented Jan 28, 2021

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

TomRoSystems commented Jan 28, 2021

lalitb commented Jan 28, 2021

TomRoSystems commented Jan 28, 2021

codecov bot commented Jan 19, 2021 •

edited

Loading

lalitb left a comment •

edited

Loading

lalitb Jan 20, 2021 •

edited

Loading

lalitb Jan 20, 2021 •

edited

Loading

lalitb Jan 26, 2021 •

edited

Loading

TomRoSystems Jan 27, 2021 •

edited

Loading