Add functionality to explicitely specify a span parent #349

pyohannes · 2020-10-02T00:34:35Z

This PR extends the StartSpanOptions struct by a parent SpanContext. If this is given, it will be used as a parent for a given span.

While I was at it, I cleaned up some things in the SpanContext implementation.

Closes #318.

pyohannes · 2020-10-02T00:36:04Z

api/include/opentelemetry/trace/span_context.h

@@ -33,10 +33,6 @@ namespace trace_api = opentelemetry::trace;
 class SpanContext final
 {
 public:
-  // An invalid SpanContext.
-  SpanContext() noexcept


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.

Explicit delete as SpanContext() = delete;?

Good call, I made it explicit.

pyohannes · 2020-10-02T00:37:11Z

api/include/opentelemetry/trace/span_context.h

+  trace_api::TraceId trace_id_;
+  trace_api::SpanId span_id_;
+  trace_api::TraceFlags trace_flags_;
+  bool remote_parent_ = false;


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

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

codecov · 2020-10-02T00:40:16Z

Codecov Report

Merging #349 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   94.71%   94.73%   +0.01%     
==========================================
  Files         154      154              
  Lines        6847     6871      +24     
==========================================
+ Hits         6485     6509      +24     
  Misses        362      362

Impacted Files	Coverage Δ
api/include/opentelemetry/trace/default_span.h	`21.42% <ø> (ø)`
api/include/opentelemetry/trace/span.h	`100.00% <ø> (ø)`
api/include/opentelemetry/trace/span_context.h	`100.00% <ø> (ø)`
api/include/opentelemetry/trace/noop.h	`86.20% <100.00%> (+1.02%)`	⬆️
api/test/trace/span_context_test.cc	`100.00% <100.00%> (ø)`
sdk/src/trace/tracer.cc	`73.52% <100.00%> (+2.56%)`	⬆️
sdk/test/trace/tracer_test.cc	`100.00% <100.00%> (ø)`
sdk/test/trace/span_data_test.cc	`100.00% <0.00%> (ø)`

ThomsonTan · 2020-10-02T01:01:24Z

api/include/opentelemetry/trace/span.h

+  //
+  // This defaults to an invalid span context. In this case, the Span is
+  // automatically parented to the currently active span.
+  SpanContext parent = SpanContext::GetInvalid();


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

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.

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.

lalitb · 2020-10-02T15:42:23Z

api/include/opentelemetry/trace/span_context.h

-  //    this = spn_ctx;
-  //    return *this;
-  //  };
+  SpanContext &operator=(const SpanContext &ctx) = default;


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

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.

lalitb · 2020-10-02T15:45:47Z

api/include/opentelemetry/trace/span_context.h

+  trace_api::TraceId trace_id_;
+  trace_api::SpanId span_id_;
+  trace_api::TraceFlags trace_flags_;
+  bool remote_parent_ = false;


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

lalitb · 2020-10-02T16:15:22Z

api/include/opentelemetry/trace/span.h

+  //
+  // This defaults to an invalid span context. In this case, the Span is
+  // automatically parented to the currently active span.
+  SpanContext parent = SpanContext::GetInvalid();


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.

lalitb · 2020-10-02T16:16:05Z

api/include/opentelemetry/trace/span.h

+  //
+  // This defaults to an invalid span context. In this case, the Span is
+  // automatically parented to the currently active span.
+  SpanContext parent = SpanContext::GetInvalid();


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.

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.

Johannes Tax added 3 commits September 30, 2020 20:03

Add parent context

10cc687

Explicit span parenting

3ba1346

Cleanup

276963f

pyohannes requested a review from a team October 2, 2020 00:34

pyohannes commented Oct 2, 2020

View reviewed changes

Fix compilation

d63c344

Fix compilation

6e54d16

pyohannes changed the title ~~Add functionality to explicitely specify a span parent~~ [WIP] Add functionality to explicitely specify a span parent Oct 2, 2020

ThomsonTan reviewed Oct 2, 2020

View reviewed changes

Fix build issues

f44f603

pyohannes changed the title ~~[WIP] Add functionality to explicitely specify a span parent~~ Add functionality to explicitely specify a span parent Oct 2, 2020

lalitb reviewed Oct 2, 2020

View reviewed changes

lalitb approved these changes Oct 7, 2020

View reviewed changes

ThomsonTan approved these changes Oct 7, 2020

View reviewed changes

reyang mentioned this pull request Oct 7, 2020

Add lalitb and ThomsonTan as approvers #355

Merged

pyohannes added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Oct 8, 2020

Merge branch 'master' into explicit-parent

ecddbb8

reyang merged commit 4a1b793 into open-telemetry:master Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to explicitely specify a span parent #349

Add functionality to explicitely specify a span parent #349

pyohannes commented Oct 2, 2020 •

edited

Loading

pyohannes Oct 2, 2020

ThomsonTan Oct 2, 2020

pyohannes Oct 2, 2020

pyohannes Oct 2, 2020

lalitb Oct 2, 2020

codecov bot commented Oct 2, 2020 •

edited

Loading

ThomsonTan Oct 2, 2020 •

edited

Loading

pyohannes Oct 2, 2020

lalitb Oct 2, 2020

lalitb Oct 2, 2020

pyohannes Oct 2, 2020

lalitb Oct 2, 2020

lalitb Oct 2, 2020

lalitb Oct 2, 2020 •

edited

Loading

pyohannes Oct 2, 2020

Add functionality to explicitely specify a span parent #349

Add functionality to explicitely specify a span parent #349

Conversation

pyohannes commented Oct 2, 2020 • 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

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2020 • edited Loading

Codecov Report

ThomsonTan Oct 2, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyohannes commented Oct 2, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

ThomsonTan Oct 2, 2020 •

edited

Loading

lalitb Oct 2, 2020 •

edited

Loading