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

Add composite propagator #597

Merged
merged 9 commits into from
Mar 9, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Increment the:
## [Unreleased]

* [EXPORTER] Added Zipkin Exporter. ([#471](https://github.com/open-telemetry/opentelemetry-cpp/pull/471))
* [PROPAGATOR] Added Composite Propagator ([#597](https://github.com/open-telemetry/opentelemetry-cpp/pull/597))

## [0.2.0] 2021-03-02

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#include <initializer_list>
#include <memory>
#include <vector>
#include "opentelemetry/trace/propagation/text_map_propagator.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace trace
{
namespace propagation
{

template <typename T>
class CompositePropagator : public TextMapPropagator<T>
{
public:
CompositePropagator(std::vector<std::shared_ptr<TextMapPropagator<T>>> &propagators)
Copy link
Contributor

Choose a reason for hiding this comment

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

A few minor nitpicks:

  • Should CompositePropagator own the propagators? i.e. use unique_ptr over shared_ptr?
  • Pass by value and std::move?
  • Would it help to add a method to add a propagator to composite propagator?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Should CompositePropagator own the propagators? i.e. use unique_ptr over shared_ptr?
  • Pass by value and std::move?

Very valid points. I can't think of scenario where we need to share Propagator across. Will fix this, unless someone further comments with some scenario.

  • Would it help to add a method to add a propagator to composite propagator?

This should help if we need to add propagator after creating the initial composite object. Specs ( https://github.com/open-telemetry/opentelemetry-specification/blob/14d123c121b6caa53bffd011292c42a181c9ca26/specification/context/api-propagators.md#composite-propagator ) doesn't ask for it explicitly so would keep it as it for now. We can add later if required.

Copy link
Member Author

Choose a reason for hiding this comment

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

First two points are implemented now.

: propagators_(propagators)
{}
// Rules that manages how context will be extracted from carrier.
using Getter = nostd::string_view (*)(const T &carrier, nostd::string_view trace_type);

// 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);

/**
* Run each of the configured propagators with the given context and carrier.
* Propagators are run in the order they are configured, so if multiple
* propagators write the same carrier key, the propagator later in the list
* will "win".
*
* @param setter Rules that manages how context will be injected to carrier.
* @param carrier Carrier into which context will be injected
* @param context Context to inject
*
*/

void Inject(Setter setter, T &carrier, const context::Context &context) noexcept override
{
for (auto &p : propagators_)
{
p->Inject(setter, carrier, context);
}
}

/**
* Run each of the configured propagators with the given context and carrier.
* Propagators are run in the order they are configured, so if multiple
* propagators write the same context key, the propagator later in the list
* will "win".
*
* @param setter Rules that manages how context will be extracte from carrier.
* @param context Context to add values to
* @param carrier Carrier from which to extract context
*/
context::Context Extract(Getter getter,
const T &carrier,
context::Context &context) noexcept override
{
for (auto &p : propagators_)
{
context = p->Extract(getter, carrier, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems no need to assign all intermediate extracted context to input context. Probably create temp context::Context to hold the extracted context and return the last one?

Is it possible to get empty propagators_ list, then return the input parameter context? Do we need initialize the context to empty in such case?

Copy link
Member Author

@lalitb lalitb Mar 9, 2021

Choose a reason for hiding this comment

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

There seems no need to assign all intermediate extracted context to input context. Probably create temp context::Context to hold the extracted context and return the last one?

Good point - I did implement this initially, but then later discovered that all other languages implementations are modifying the input context and so reverted. Specs doesn't say anything about it, so was unsure. Have changed it back as you suggested now. Let me know if you see any concerns here :)

Is it possible to get empty propagators_ list, then return the input parameter context? Do we need initialize the context to empty in such case?

Yes it's possible to get empty list, and we should be returning the input context unchanged it that case.

}
return context;
}

private:
std::vector<std::shared_ptr<TextMapPropagator<T>>> propagators_;
};
} // namespace propagation
} // namespace trace
OPENTELEMETRY_END_NAMESPACE;
3 changes: 2 additions & 1 deletion api/test/trace/propagation/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
foreach(testname http_text_format_test b3_propagation_test)
foreach(testname http_text_format_test b3_propagation_test
composite_propagator_test)
add_executable(${testname} "${testname}.cc")
target_link_libraries(
${testname} ${GTEST_BOTH_LIBRARIES} ${CORE_RUNTIME_LIBS}
Expand Down
122 changes: 122 additions & 0 deletions api/test/trace/propagation/composite_propagator_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
#include "opentelemetry/context/context.h"
#include "opentelemetry/nostd/shared_ptr.h"
#include "opentelemetry/nostd/span.h"
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/trace/default_span.h"
#include "opentelemetry/trace/noop.h"
#include "opentelemetry/trace/span.h"
#include "opentelemetry/trace/span_context.h"
#include "opentelemetry/trace/trace_id.h"
#include "opentelemetry/trace/tracer.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these includes needed or only some of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all, will remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


#include <map>
#include <memory>
#include <string>

#include <gtest/gtest.h>

#include "opentelemetry/trace/default_span.h"
lalitb marked this conversation as resolved.
Show resolved Hide resolved
#include "opentelemetry/trace/propagation/b3_propagator.h"
#include "opentelemetry/trace/propagation/composite_propagator.h"
#include "opentelemetry/trace/propagation/http_trace_context.h"
#include "opentelemetry/trace/propagation/text_map_propagator.h"

using namespace opentelemetry;

template <typename T>
static std::string Hex(const T &id_item)
{
char buf[T::kSize * 2];
id_item.ToLowerBase16(buf);
return std::string(buf, sizeof(buf));
}

static nostd::string_view Getter(const std::map<std::string, std::string> &carrier,
nostd::string_view trace_type)
{
auto it = carrier.find(std::string(trace_type));
if (it != carrier.end())
{
return nostd::string_view(it->second);
}
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to return nullptr here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will need change across all propagators. We can do it separately if needed, not in current scope of this PR.

}

static void Setter(std::map<std::string, std::string> &carrier,
nostd::string_view trace_type,
nostd::string_view trace_description = "")
{
carrier[std::string(trace_type)] = std::string(trace_description);
}

class CompositeTestPropagatorTest : public ::testing::Test
{
using MapHttpTraceContext =
trace::propagation::HttpTraceContext<std::map<std::string, std::string>>;

using MapB3Context = trace::propagation::B3Propagator<std::map<std::string, std::string>>;
using MapCompositePropagator =
trace::propagation::CompositePropagator<std::map<std::string, std::string>>;

public:
CompositeTestPropagatorTest()
: tc_propogator_(std::make_shared<MapHttpTraceContext>()),
b3_propogator_(std::make_shared<MapB3Context>())
{
std::vector<
std::shared_ptr<trace::propagation::TextMapPropagator<std::map<std::string, std::string>>>>
propogator_list = {};
propogator_list.push_back(tc_propogator_);
propogator_list.push_back(b3_propogator_);

composite_propagator_ = new MapCompositePropagator(propogator_list);
}

~CompositeTestPropagatorTest() { delete composite_propagator_; }

protected:
std::shared_ptr<MapHttpTraceContext> tc_propogator_;
std::shared_ptr<MapB3Context> b3_propogator_;
MapCompositePropagator *composite_propagator_;
};

TEST_F(CompositeTestPropagatorTest, Extract)
{

const std::map<std::string, std::string> carrier = {
{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
{"b3", "80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1-1-05e3ac9a4f6e3b90"}};
context::Context ctx1 = context::Context{};

context::Context ctx2 = composite_propagator_->Extract(Getter, carrier, ctx1);

auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
EXPECT_TRUE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));

auto span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);

// confirm last propagator in composite propagator list ( B3 here) wins for same key
lalitb marked this conversation as resolved.
Show resolved Hide resolved
// ("active_span" here).
EXPECT_EQ(Hex(span->GetContext().trace_id()), "80f198ee56343ba864fe8b2a57d3eff7");
EXPECT_EQ(Hex(span->GetContext().span_id()), "e457b5a2e4d86bd1");
EXPECT_EQ(span->GetContext().IsSampled(), true);
EXPECT_EQ(span->GetContext().IsRemote(), true);
}

TEST_F(CompositeTestPropagatorTest, Inject)
{

constexpr uint8_t buf_span[] = {1, 2, 3, 4, 5, 6, 7, 8};
constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
trace::TraceFlags{true}, false};
nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};

// Set `sp` as the currently active span, which must be used by `Inject`.
trace::Scope scoped_span{sp};

std::map<std::string, std::string> headers = {};
composite_propagator_->Inject(Setter, headers, context::RuntimeContext::GetCurrent());
EXPECT_EQ(headers["traceparent"], "00-0102030405060708090a0b0c0d0e0f10-0102030405060708-01");
EXPECT_EQ(headers["b3"], "0102030405060708090a0b0c0d0e0f10-0102030405060708-1");
}