Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding B3 propagator #523

Merged
merged 10 commits into from
Jan 28, 2021
14 changes: 14 additions & 0 deletions api/include/opentelemetry/nostd/string_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = 0const noexcept;
constexpr size_type find(CharT ch, size_type pos = 0const 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 = 0const;

Copy link
Member Author

@TomRoSystems TomRoSystems Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
size_type res = npos;
if (pos < length())
{
auto found = Traits::find(data() + pos, length() - pos, ch);
if (found)
{
res = found - data();
}
}
return res;
}

bool operator<(const string_view v) const noexcept { return compare(v) < 0; }

bool operator>(const string_view v) const noexcept { return compare(v) > 0; }
Expand Down
276 changes: 276 additions & 0 deletions api/include/opentelemetry/trace/propagation/b3_propagator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,276 @@
#pragma once

#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"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace trace
{
namespace propagation
{

static const nostd::string_view kB3CombinedHeader = "b3";

static const nostd::string_view kB3TraceIdHeader = "X-B3-TraceId";
static const nostd::string_view kB3SpanIdHeader = "X-B3-SpanId";
static const nostd::string_view kB3SampledHeader = "X-B3-Sampled";

/*
B3, single header:
b3: 80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1-1-05e3ac9a4f6e3b90
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^
0 TraceId 31 33 SpanId 48 | 52 ParentSpanId 68
50 Debug flag
Multiheader version: X-B3-Sampled
X-B3-TraceId X-B3-SpanId X-B3-ParentSpanId (ignored)
*/

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

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space after object.

// Based on:
// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#b3-extract
template <typename T>
class B3PropagatorExtractor : public HTTPTextFormat<T>
{
public:
// Rules that manages how context will be extracted from carrier.
using Getter = nostd::string_view (*)(const T &carrier, nostd::string_view trace_type);

// Returns the context that is stored in the HTTP header carrier with the getter as extractor.
context::Context Extract(Getter getter,
const T &carrier,
context::Context &context) noexcept override
{
SpanContext span_context = ExtractImpl(getter, carrier);
nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
return context.SetValue(kSpanKey, sp);
}

static SpanContext GetCurrentSpan(const context::Context &context)
{
context::Context ctx(context);
context::ContextValue span = ctx.GetValue(kSpanKey);
if (nostd::holds_alternative<nostd::shared_ptr<Span>>(span))
{
return nostd::get<nostd::shared_ptr<Span>>(span).get()->GetContext();
}
return SpanContext::GetInvalid();
}

static TraceId GenerateTraceIdFromString(nostd::string_view trace_id)
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
uint8_t buf[kTraceIdHexStrLength / 2];
uint8_t *b_ptr = buf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b_ptr is unused now?

GenerateBuffFromHexStrPad0(trace_id, sizeof(buf), buf);
return TraceId(buf);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

GenerateBuffFromHexStrPad0(span_id, sizeof(buf), buf);
return SpanId(buf);
}

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

private:
// Converts hex numbers (string_view) into bytes stored in a buffer and pads buffer with 0.
static void GenerateBuffFromHexStrPad0(nostd::string_view hexStr, int bufSize, uint8_t *buf)
{ // we are doing this starting from "right" side for left-padding
nostd::string_view::size_type posInp = hexStr.length();
int posOut = bufSize;
while (posOut--)
{
int val = 0;
if (posInp)
{
int hexDigit2 = HexToInt(hexStr[--posInp]); // low nibble
int hexDigit1 = 0;
if (posInp)
{
hexDigit1 = HexToInt(hexStr[--posInp]);
}
if (hexDigit1 < 0 || hexDigit2 < 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :( copied from here...

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

{ // malformed hex sequence. Fill entire buffer with zeroes.
for (int j = 0; j < bufSize; j++)
{
buf[j] = 0;
}
return;
}
val = hexDigit1 * 16 + hexDigit2;
}
buf[posOut] = val;
}
}

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
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 SpanContext ExtractImpl(Getter getter, const T &carrier)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lalitb lalitb Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// all these are hex values
nostd::string_view trace_id;
nostd::string_view span_id;
nostd::string_view trace_flags;

// first let's try a single-header variant
auto singleB3Header = getter(carrier, kB3CombinedHeader);
if (!singleB3Header.empty())
{
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (firstSep != nostd::string_view::npos)
{ // at least two fields are required
auto secondSep = singleB3Header.find('-', firstSep + 1);
if (secondSep != nostd::string_view::npos)
{ // more than two fields - check also trace_flags
span_id = singleB3Header.substr(firstSep + 1, secondSep - firstSep - 1);
if (secondSep + 1 < singleB3Header.size())
{
trace_flags = singleB3Header.substr(secondSep + 1, kTraceFlagHexStrLength);
}
}
else
{
span_id = singleB3Header.substr(firstSep + 1);
}
}
}
else
{
trace_id = getter(carrier, kB3TraceIdHeader);
span_id = getter(carrier, kB3SpanIdHeader);
trace_flags = getter(carrier, kB3SampledHeader);
}

// now convert hex to objects
TraceId trace_id_obj = GenerateTraceIdFromString(trace_id);
SpanId span_id_obj = GenerateSpanIdFromString(span_id);
if (!trace_id_obj.IsValid() || !span_id_obj.IsValid())
{
return SpanContext(false, false);
}
TraceFlags trace_flags_obj = GenerateTraceFlagsFromString(trace_flags);
return SpanContext(trace_id_obj, span_id_obj, trace_flags_obj, true);
}
};

// The B3Propagator class provides interface that enables extracting and injecting context into
// single header of HTTP Request.
template <typename T>
class B3Propagator : public B3PropagatorExtractor<T>
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
public:
// Rules that manages how context will be injected to carrier.
using Setter = void (*)(T &carrier,
nostd::string_view trace_type,
nostd::string_view trace_description);
// Sets the context for a HTTP header carrier with self defined rules.
void Inject(Setter setter, T &carrier, const context::Context &context) noexcept override
{
SpanContext span_context = B3PropagatorExtractor<T>::GetCurrentSpan(context);
if (!span_context.IsValid())
{
return;
}
char trace_id[32];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TraceId(span_context.trace_id()).ToLowerBase16(trace_id);
char span_id[16];
SpanId(span_context.span_id()).ToLowerBase16(span_id);
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 = "";
for (int i = 0; i < 32; i++)
{
hex_string.push_back(trace_id[i]);
}
hex_string.push_back('-');
for (int i = 0; i < 16; i++)
{
hex_string.push_back(span_id[i]);
}
hex_string.push_back('-');
hex_string.push_back(trace_flags[1]);
setter(carrier, kB3CombinedHeader, hex_string);
}
};

template <typename T>
class B3PropagatorMultiHeader : public B3PropagatorExtractor<T>
{
public:
// Rules that manages how context will be injected to carrier.
using Setter = void (*)(T &carrier,
nostd::string_view trace_type,
nostd::string_view trace_description);
void Inject(Setter setter, T &carrier, const context::Context &context) noexcept override
{
SpanContext span_context = B3PropagatorExtractor<T>::GetCurrentSpan(context);
if (!span_context.IsValid())
{
return;
}
char trace_id[32];
TraceId(span_context.trace_id()).ToLowerBase16(trace_id);
char span_id[16];
SpanId(span_context.span_id()).ToLowerBase16(span_id);
char trace_flags[2];
TraceFlags(span_context.trace_flags()).ToLowerBase16(trace_flags);
setter(carrier, kB3TraceIdHeader, nostd::string_view(trace_id, sizeof(trace_id)));
setter(carrier, kB3SpanIdHeader, nostd::string_view(span_id, sizeof(span_id)));
setter(carrier, kB3SampledHeader, nostd::string_view(trace_flags + 1, 1));
}
};

} // namespace propagation
} // namespace trace
OPENTELEMETRY_END_NAMESPACE
18 changes: 18 additions & 0 deletions api/test/nostd/string_view_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,24 @@ TEST(StringViewTest, SubstrOutOfRange)
#endif
}

TEST(StringViewTest, FindSingleCharacter)
{
string_view s = "abc";

// starting from 0-th position (default)
EXPECT_EQ(s.find('a'), 0);
EXPECT_EQ(s.find('b'), 1);
EXPECT_EQ(s.find('c'), 2);
EXPECT_EQ(s.find('d'), -1); // FIXME: string_view:npos - problem with linker

// starting from given index
EXPECT_EQ(s.find('a', 1), -1);
EXPECT_EQ(s.find('b', 1), 1);

// out of index
EXPECT_EQ(s.find('a', 10), -1);
}

TEST(StringViewTest, Compare)
{
string_view s1 = "aaa";
Expand Down
2 changes: 1 addition & 1 deletion api/test/trace/propagation/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
foreach(testname http_text_format_test)
foreach(testname http_text_format_test b3_propagation_test)
add_executable(${testname} "${testname}.cc")
target_link_libraries(
${testname} ${GTEST_BOTH_LIBRARIES} ${CORE_RUNTIME_LIBS}
Expand Down
Loading