-
Notifications
You must be signed in to change notification settings - Fork 440
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
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4d798b0
add composite propagator
lalitb d1d4430
add new line
lalitb 10aa6eb
Merge branch 'main' into composite-propagator
lalitb c4314cf
add changelog
lalitb e9f1844
Update api/test/trace/propagation/composite_propagator_test.cc
lalitb f4aaee6
review comments
lalitb 4eb356c
fix
lalitb c672eaf
format fix
lalitb 7c52542
add flag
lalitb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
73 changes: 73 additions & 0 deletions
73
api/include/opentelemetry/trace/propagation/composite_propagator.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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::unique_ptr<TextMapPropagator<T>>> propagators) | ||
: propagators_(std::move(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); | ||
} | ||
return context; | ||
} | ||
|
||
private: | ||
std::vector<std::unique_ptr<TextMapPropagator<T>>> propagators_; | ||
}; | ||
} // namespace propagation | ||
} // namespace trace | ||
OPENTELEMETRY_END_NAMESPACE; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
120 changes: 120 additions & 0 deletions
120
api/test/trace/propagation/composite_propagator_test.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/trace/scope.h" | ||
#include "opentelemetry/trace/span.h" | ||
#include "opentelemetry/trace/span_context.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" | ||
|
||
#include <map> | ||
#include <memory> | ||
#include <string> | ||
|
||
#include <gtest/gtest.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 ""; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it better to return There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
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>>; | ||
|
||
class CompositePropagatorTest : 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: | ||
CompositePropagatorTest() | ||
{ | ||
std::vector< | ||
std::unique_ptr<trace::propagation::TextMapPropagator<std::map<std::string, std::string>>>> | ||
propogator_list = {}; | ||
std::unique_ptr<MapHttpTraceContext> tc_propogator(new MapHttpTraceContext()); | ||
std::unique_ptr<MapB3Context> b3_propogator(new MapB3Context()); | ||
propogator_list.push_back(std::move(tc_propogator)); | ||
propogator_list.push_back(std::move(b3_propogator)); | ||
|
||
composite_propagator_ = new MapCompositePropagator(std::move(propogator_list)); | ||
} | ||
|
||
~CompositePropagatorTest() { delete composite_propagator_; } | ||
|
||
protected: | ||
MapCompositePropagator *composite_propagator_; | ||
}; | ||
|
||
TEST_F(CompositePropagatorTest, 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 | ||
// ("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(CompositePropagatorTest, 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"); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
Yes it's possible to get empty list, and we should be returning the input context unchanged it that case.