Adding B3 propagator #1

TomRoSystems · 2021-01-18T23:42:28Z

No description provided.

bogdandrutu · 2021-01-19T17:13:26Z

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

+#include <array>
+#include <iostream>
+#include <map>
+#include <string>
+#include "opentelemetry/common/key_value_iterable.h"
+#include "opentelemetry/context/context.h"
+#include "opentelemetry/nostd/shared_ptr.h"
+#include "opentelemetry/nostd/span.h"
+#include "opentelemetry/nostd/string_view.h"
+#include "opentelemetry/nostd/variant.h"
+#include "opentelemetry/trace/default_span.h"
+#include "opentelemetry/trace/propagation/http_text_format.h"
+#include "opentelemetry/trace/span.h"
+#include "opentelemetry/trace/span_context.h"


Order of includes, use clang to format

This PR is running same set of checks as opentelemtry-cpp and clang formatting is OK:
https://github.com/TomRoSystems/opentelemetry-cpp/pull/1/checks?check_run_id=1724608707

bogdandrutu · 2021-01-19T17:14:25Z

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

+static const nostd::string_view B3CombinedHeader = "b3";
+
+static const nostd::string_view B3TraceIdHeader = "X-B3-TraceId";
+static const nostd::string_view B3SpanIdHeader  = "X-B3-SpanId";
+static const nostd::string_view B3SampledHeader = "X-B3-Sampled";


Do you need this to be public and exported?

bogdandrutu · 2021-01-19T17:14:56Z

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

+static const int kTraceIdHexStrLength   = 32;
+static const int kSpanIdHexStrLength    = 16;
+static const int kTraceFlagHexStrLength = 1;


Same question for these values, if not we can move them to the cc file.

This file was highly inspired by https://github.com/open-telemetry/opentelemetry-cpp/blob/880e392e10b6d8e964ade2e22d7231950d6d2443/api/include/opentelemetry/trace/propagation/http_trace_context.h and all API in C++ is header-only so there's no cc file.

bogdandrutu · 2021-01-19T17:32:32Z

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;
+    GenerateHexFromString(trace_id, kTraceIdHexStrLength, b_ptr);
+    return TraceId(buf);
+  }
+
+  static SpanId GenerateSpanIdFromString(nostd::string_view span_id)
+  {
+    uint8_t buf[kSpanIdHexStrLength / 2];
+    uint8_t *b_ptr = buf;
+    GenerateHexFromString(span_id, kSpanIdHexStrLength, b_ptr);
+    return SpanId(buf);
+  }


No need to be public correct?

Same as with https://github.com/open-telemetry/opentelemetry-cpp/blob/880e392e10b6d8e964ade2e22d7231950d6d2443/api/include/opentelemetry/trace/propagation/http_trace_context.h - I think it's public for testing purposes.

bogdandrutu · 2021-01-19T17:32:39Z

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

+  static TraceFlags GenerateTraceFlagsFromString(nostd::string_view trace_flags)
+  {
+    if (trace_flags.length() != 1 || (trace_flags[0] != '1' && trace_flags[0] != 'd'))
+    {
+      return TraceFlags(0);  // check for invalid length of flags
+    }
+    return TraceFlags(TraceFlags::kIsSampled);  // treat 'd' as kIsSampled
+  }


seemk · 2021-01-20T08:10:05Z

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

+  // Converts the hex numbers stored as strings into bytes stored in a buffer.
+  static void GenerateHexFromString(nostd::string_view string, int bytes, uint8_t *buf)
+  {
+    const char *str_id = string.data();
+    for (int i = 0; i < bytes; i++)
+    {
+      int tmp = HexToInt(str_id[i]);
+      if (tmp < 0)
+      {
+        for (int j = 0; j < bytes / 2; j++)
+        {
+          buf[j] = 0;
+        }
+        return;
+      }
+      if (i % 2 == 0)
+      {
+        buf[i / 2] = tmp * 16;
+      }
+      else
+      {
+        buf[i / 2] += tmp;
+      }
+    }
+  }
+
+  // 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)
+  {
+    if (c >= '0' && c <= '9')
+    {
+      return (int)(c - '0');
+    }
+    else if (c >= 'a' && c <= 'f')
+    {
+      return (int)(c - 'a' + 10);
+    }
+    else if (c >= 'A' && c <= 'F')
+    {
+      return (int)(c - 'A' + 10);
+    }
+    else
+    {
+      return -1;
+    }
+  }
+
+  static bool IsValidHex(nostd::string_view string_view)
+  {
+    for (int i = 0; i < string_view.length(); i++)
+    {
+      if (!(string_view[i] >= '0' && string_view[i] <= '9') &&
+          !(string_view[i] >= 'a' && string_view[i] <= 'f'))
+        return false;
+    }
+    return true;
+  }


Would it make sense to move these elsewhere? It's repeated here and at http_trace_context

seemk · 2021-01-20T08:13:42Z

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

+    auto singleB3Header = getter(carrier, B3CombinedHeader);
+    if (singleB3Header != "")
+    {
+      trace_id = singleB3Header.substr(0, kTraceIdHexStrLength);


B3 trace ID can be either 64 or 128 bits (that is 16 or 32 hex chars): https://github.com/openzipkin/b3-propagation#traceid-1

seemk · 2021-01-20T08:25:50Z

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

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


This would be a buffer overrun in case of 64 bit trace ids (which are unhandled currently, but a word of caution)

seemk · 2021-01-20T08:28:19Z

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

+      trace_id = singleB3Header.substr(0, kTraceIdHexStrLength);
+      if (singleB3Header.size() > 33)
+      {
+        span_id = singleB3Header.substr(33, kSpanIdHexStrLength);


What would happen if the input header size is 40?

seemk · 2021-01-20T08:34:02Z

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

+    char trace_flags[2];
+    TraceFlags(span_context.trace_flags()).ToLowerBase16(trace_flags);
+    // Note: This is only temporary replacement for appendable string
+    std::string hex_string = "";


This is on the hot path and will most likely allocate twice here when appending. Could use a stack buffer here with the appropriate size, since a string_view is passed to the setter anyway.

seemk · 2021-01-20T08:36:50Z

api/test/trace/propagation/b3_propagation_test.cc

+  EXPECT_EQ(headers["X-B3-TraceId"], "0102030405060708090a0b0c0d0e0f10");
+  EXPECT_EQ(headers["X-B3-SpanId"], "0102030405060708");
+  EXPECT_EQ(headers["X-B3-Sampled"], "1");
+}


Would be nice to have a test of left-padding an incoming 64 bit trace id with zeroes

…etry#514)

…metry#529)

…try#520)

- 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

codecov-io · 2021-01-22T19:54:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@880e392). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   94.57%           
=========================================
  Files             ?      196           
  Lines             ?     8739           
  Branches          ?        0           
=========================================
  Hits              ?     8265           
  Misses            ?      474           
  Partials          ?        0

TomRoSystems · 2021-01-29T19:00:50Z

Already merged open-telemetry#523

Adding B3 propagator

415bd5b

bogdandrutu reviewed Jan 19, 2021

View reviewed changes

OTLP change default port number (open-telemetry#522)

0435edb

seemk suggested changes Jan 20, 2021

View reviewed changes

reyang and others added 11 commits January 21, 2021 09:49

Add spell check to CI (open-telemetry#527)

ab7f5f2

Update PR merge requirement. (open-telemetry#524)

1a55ef1

fix namespace in trace_state class (open-telemetry#526)

c4df3bf

Fix grpc_cpp_plugin executable import (open-telemetry#525)

a9ca938

Add a test service for the W3C validation harness (open-telemetry#487)

4a13db9

Fix missing include dir for configure_package_config_file (open-telem…

d65d7c0

…etry#514)

Rename SpanContext.HasRemoteParent to SpanContext.IsRemote (open-tele…

51eef56

…metry#529)

Fix calling target_include_directories on library folder (open-teleme…

ad9e57f

…try#520)

Resource sdk Implementation (open-telemetry#502)

ce89aa1

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

TomRoSystems and others added 13 commits January 22, 2021 21:34

Make traceId left-padded

37269ee

Merge branch 'master' into b3_propagator

11ac503

Rename SpanContext.HasRemoteParent to SpanContext.IsRemote

8c78d51

Fix for condition always false + test for invalid hex value

bb74736

Proper fix with latest master branch

643602b

Move from master to main (step1) (open-telemetry#531)

484700a

finish master to main migration (open-telemetry#533)

47d6554

Added Zoom passcode (open-telemetry#532)

8fd86ed

Sending sync request through http_client (open-telemetry#448)

7c6220a

Fix install location of CMake config script (open-telemetry#528)

ee79464

Improve invalid span Id check (open-telemetry#538)

d94a368

Remove unused variables. Use constants for array sizes.

1a8bb3c

Merge branch 'main' into b3_propagator

f52ce7b

Base automatically changed from master to main January 29, 2021 18:58

TomRoSystems closed this Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding B3 propagator #1

Adding B3 propagator #1

TomRoSystems commented Jan 18, 2021

bogdandrutu Jan 19, 2021

TomRoSystems Jan 19, 2021

bogdandrutu Jan 19, 2021

bogdandrutu Jan 19, 2021

TomRoSystems Jan 19, 2021

bogdandrutu Jan 19, 2021

TomRoSystems Jan 19, 2021

bogdandrutu Jan 19, 2021

seemk Jan 20, 2021

seemk Jan 20, 2021

seemk Jan 20, 2021 •

edited

Loading

seemk Jan 20, 2021

seemk Jan 20, 2021

seemk Jan 20, 2021

codecov-io commented Jan 22, 2021 •

edited

Loading

TomRoSystems commented Jan 29, 2021

Adding B3 propagator #1

Adding B3 propagator #1

Conversation

TomRoSystems commented Jan 18, 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

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

seemk Jan 20, 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

codecov-io commented Jan 22, 2021 • edited Loading

Codecov Report

TomRoSystems commented Jan 29, 2021

seemk Jan 20, 2021 •

edited

Loading

codecov-io commented Jan 22, 2021 •

edited

Loading