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.

Sign up for GitHub

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 functionality to explicitely specify a span parent #349

Merged
merged 7 commits into from
Oct 9, 2020
Merged

Add functionality to explicitely specify a span parent #349

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
10cc687
Add parent context
Oct 1, 2020
3ba1346
Explicit span parenting
Oct 2, 2020
276963f
Cleanup
Oct 2, 2020
d63c344
Fix compilation
Oct 2, 2020
6e54d16
Fix compilation
Oct 2, 2020
f44f603
Fix build issues
Oct 2, 2020
ecddbb8
Merge branch 'master' into explicit-parent
Oct 8, 2020
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
2 changes: 0 additions & 2 deletions api/include/opentelemetry/trace/default_span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class DefaultSpan : public Span

nostd::string_view ToString() { return "DefaultSpan"; }

DefaultSpan() = default;

DefaultSpan(SpanContext span_context) : span_context_(span_context) {}

// movable and copiable
Expand Down
4 changes: 3 additions & 1 deletion api/include/opentelemetry/trace/noop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ namespace trace
class NoopSpan final : public Span
{
public:
explicit NoopSpan(const std::shared_ptr<Tracer> &tracer) noexcept : tracer_{tracer} {}
explicit NoopSpan(const std::shared_ptr<Tracer> &tracer) noexcept
: tracer_{tracer}, span_context_{SpanContext::GetInvalid()}
{}

void SetAttribute(nostd::string_view /*key*/,
const common::AttributeValue & /*value*/) noexcept override
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/trace/propagation/http_trace_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class HttpTraceContext : public HTTPTextFormat<T>
{
return nostd::get<nostd::shared_ptr<Span>>(span).get()->GetContext();
}
return SpanContext();
return SpanContext::GetInvalid();
}

static TraceId GenerateTraceIdFromString(nostd::string_view trace_id)
Expand Down
7 changes: 6 additions & 1 deletion api/include/opentelemetry/trace/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,13 @@ struct StartSpanOptions
core::SystemTimestamp start_system_time;
core::SteadyTimestamp start_steady_time;

// Explicitely set the parent of a Span.
//
// This defaults to an invalid span context. In this case, the Span is
// automatically parented to the currently active span.
SpanContext parent = SpanContext::GetInvalid();
Copy link
Contributor

@ThomsonTan ThomsonTan Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Defaults to invalid value may look a little weird, would GetEmpty() be a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that, but then decided against introducing an empty span context. In the current terminology, a span context can either be valid or invalid (it has an IsValid method). So SpanContext::GetInvalid()::IsValid() == true I think is more idiomatic than SpanContext::GetEmpty()::IsValid() == true. And here, an invalid span context is used like a null pointer (indicating that no parent span context is set).

But I don't have a very strong opinion on that, I'd be willing to go with GetEmpty() if folks feel that's clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I too don't have strong opinion on this, although having two different methods GetEmpty() and GetInvalid() returning invalid span context is more confusing to me.

Copy link
Member

@lalitb lalitb Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just wondering if it would be more optimized to have SpanContext::GetInvalid() method returning a static instance of invalid context ( using c++11 local atomic static ). Although, Can be part of separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be more efficient if we'd return a pointer, but as we return by value here this involves a copy anyway, and we wouldn't gain much by having a static singleton.


// TODO:
// Span(Context?) parent;
// SpanContext remote_parent;
// Links
SpanKind kind = SpanKind::kInternal;
Expand Down
38 changes: 6 additions & 32 deletions api/include/opentelemetry/trace/span_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ namespace trace_api = opentelemetry::trace;
class SpanContext final
{
public:
// An invalid SpanContext.
SpanContext() noexcept
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's use SpanContext::GetInvalid() as the default way to create an invalid SpanContext. Having both SpanContext() and SpanContext::GetInvalid() returning an invalid SpanContext is somewhat confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit delete as SpanContext() = delete;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I made it explicit.

: trace_flags_(trace::TraceFlags((uint8_t) false)), remote_parent_(false){};

/* A temporary constructor for an invalid SpanContext.
* Trace id and span id are set to invalid (all zeros).
*
Expand Down Expand Up @@ -71,31 +67,9 @@ class SpanContext final
remote_parent_(has_remote_parent)
{}

SpanContext(SpanContext &&ctx)
: trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags())
{}
SpanContext(const SpanContext &ctx) = default;

SpanContext(const SpanContext &ctx)
: trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags())
{}
//
// SpanContext &operator=(const SpanContext &ctx)
// {
// SpanContext *spn_ctx =
// new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(),
// ctx.HasRemoteParent());
// this = spn_ctx;
// return *this;
// };
//
// SpanContext &operator=(SpanContext &&ctx)
// {
// SpanContext *spn_ctx =
// new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(),
// ctx.HasRemoteParent());
// this = spn_ctx;
// return *this;
// };
SpanContext &operator=(const SpanContext &ctx) = default;
Copy link
Member

Choose a reason for hiding this comment

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

Span context should be immutable as per specs. Just wondering if we need to have assignment operator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having an assignment operator makes it easier and more efficient to specify a parent context in the StartSpanOptions struct:

trace_api::StartSpanOptions options;
options.parent   = span_first->GetContext();

Without an assignment operator, we'd need to resort to a pointer (a heap allocated SpanContext object), which is more inefficient (as it requires a heap allocation/deallocation).

The SpanContext is immutable in the sense, that it has no setters (span id, trace id and flags cannot be changed independently of each other), and the SpanContext assigned to a Span cannot change either.


bool operator==(const SpanContext &that) const noexcept
{
Expand All @@ -110,10 +84,10 @@ class SpanContext final
bool IsSampled() const noexcept { return trace_flags_.IsSampled(); }

private:
const trace_api::TraceId trace_id_;
const trace_api::SpanId span_id_;
const trace_api::TraceFlags trace_flags_;
const bool remote_parent_ = false;
trace_api::TraceId trace_id_;
trace_api::SpanId span_id_;
trace_api::TraceFlags trace_flags_;
bool remote_parent_ = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the const enables us to use default (and hopefully optimized) copy constructors and assignment operators.

Copy link
Member

Choose a reason for hiding this comment

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

nit: We can bring back const-ness if we delete assignment operator ? const varaibles have its own level of optimization

};
} // namespace trace
OPENTELEMETRY_END_NAMESPACE
5 changes: 3 additions & 2 deletions api/test/trace/propagation/http_text_format_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ TEST(HTTPTextFormatTest, NoSendEmptyTraceState)
// If the trace state is empty, do not set the header.
const std::map<std::string, std::string> carrier = {
{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"}};
context::Context ctx1 =
context::Context("current-span", nostd::shared_ptr<trace::Span>(new trace::DefaultSpan()));
context::Context ctx1 = context::Context{
"current-span",
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
context::Context ctx2 = format.Extract(Getter, carrier, ctx1);
std::map<std::string, std::string> c2 = {};
format.Inject(Setter, c2, ctx2);
Expand Down
2 changes: 1 addition & 1 deletion api/test/trace/span_context_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ TEST(SpanContextTest, TraceFlags)
// Test that SpanContext is invalid
TEST(SpanContextTest, Invalid)
{
SpanContext s1(false, false);
SpanContext s1 = SpanContext::GetInvalid();
EXPECT_FALSE(s1.IsValid());

// Test that trace id and span id are invalid
Expand Down
2 changes: 1 addition & 1 deletion examples/plugin/plugin/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class Span final : public trace::Span
nostd::string_view name,
const opentelemetry::trace::KeyValueIterable & /*attributes*/,
const trace::StartSpanOptions & /*options*/) noexcept
: tracer_{std::move(tracer)}, name_{name}
: tracer_{std::move(tracer)}, name_{name}, span_context_{trace::SpanContext::GetInvalid()}
{
std::cout << "StartSpan: " << name << "\n";
}
Expand Down
26 changes: 17 additions & 9 deletions sdk/src/trace/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,37 @@ std::shared_ptr<Sampler> Tracer::GetSampler() const noexcept
return sampler_;
}

// Helper function to extract the current span context from the runtime context.
// Returns an invalid span context if the runtime context doesn't contain a span.
trace_api::SpanContext GetCurrentSpanContext()
trace_api::SpanContext GetCurrentSpanContext(const trace_api::SpanContext &explicit_parent)
{
context::ContextValue curr_span_context = context::RuntimeContext::GetValue(SpanKey);
// Use the explicit parent, if it's valid.
if (explicit_parent.IsValid())
{
return explicit_parent;
}

// Use the currently active span, if there's one.
auto curr_span_context = context::RuntimeContext::GetValue(SpanKey);

if (nostd::holds_alternative<nostd::shared_ptr<trace_api::Span>>(curr_span_context))
{
auto curr_span = nostd::get<nostd::shared_ptr<trace_api::Span>>(curr_span_context);
return curr_span->GetContext();
}
return trace_api::SpanContext();

// Otherwise return an invalid SpanContext.
return trace_api::SpanContext::GetInvalid();
}

nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options) noexcept
{
trace_api::SpanContext parent = GetCurrentSpanContext(options.parent);

// TODO: replace nullptr with parent context in span context
auto sampling_result =
sampler_->ShouldSample(nullptr, trace_api::TraceId(), name, options.kind, attributes);
sampler_->ShouldSample(nullptr, parent.trace_id(), name, options.kind, attributes);
if (sampling_result.decision == Decision::DROP)
{
// Don't allocate a no-op span for every DROP decision, but use a static
Expand All @@ -63,9 +72,8 @@ nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
}
else
{
auto span = nostd::shared_ptr<trace_api::Span>{
new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes,
options, GetCurrentSpanContext()}};
auto span = nostd::shared_ptr<trace_api::Span>{new (std::nothrow) Span{
this->shared_from_this(), processor_.load(), name, attributes, options, parent}};

// if the attributes is not nullptr, add attributes to the span.
if (sampling_result.attributes)
Expand Down
35 changes: 35 additions & 0 deletions sdk/test/trace/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,3 +442,38 @@ TEST(Tracer, WithActiveSpan)
ASSERT_EQ(1, spans.size());
EXPECT_EQ("span 1", spans.at(0)->GetName());
}

TEST(Tracer, ExpectParent)
{
std::unique_ptr<InMemorySpanExporter> exporter(new InMemorySpanExporter());
std::shared_ptr<InMemorySpanData> span_data = exporter->GetData();
auto tracer = initTracer(std::move(exporter));
auto spans = span_data.get()->GetSpans();

ASSERT_EQ(0, spans.size());

auto span_first = tracer->StartSpan("span 1");

trace_api::StartSpanOptions options;
options.parent = span_first->GetContext();
auto span_second = tracer->StartSpan("span 2", options);

options.parent = span_second->GetContext();
auto span_third = tracer->StartSpan("span 3", options);

span_third->End();
span_second->End();
span_first->End();

spans = span_data->GetSpans();
ASSERT_EQ(3, spans.size());
auto spandata_first = std::move(spans.at(2));
auto spandata_second = std::move(spans.at(1));
auto spandata_third = std::move(spans.at(0));
EXPECT_EQ("span 1", spandata_first->GetName());
EXPECT_EQ("span 2", spandata_second->GetName());
EXPECT_EQ("span 3", spandata_third->GetName());

EXPECT_EQ(spandata_first->GetSpanId(), spandata_second->GetParentSpanId());
EXPECT_EQ(spandata_second->GetSpanId(), spandata_third->GetParentSpanId());
}