tracing: Delegate envoy tracing decision to tracer to determine if should override sampling #2787

objectiser · 2018-03-13T15:24:55Z

The sampling decision is currently used to determine whether a span should be created/started. The decision is propagated to downstream services via the x-trace-id header.

The problem is that, if the x-trace-id is not propagated along with other trace context (e.g. the service does not propagate it due to using a zipkin compatible tracer in the service that does not know about this header), then the decision is lost in the downstream service and it will make its own sampling decision. This will lead to inconsistent fragmented traces.

The other problem is that any B3-sampled header received with a request will be ignored by the local sampling decision.

This PR deals with the situation by delegating the envoy proxy's tracing (sampling) decision down to the tracers themselves. There are essentially three tracers, which handle the decision in different ways:

LightStep - this tracer only creates/starts a span if the tracing decision indicates it should - otherwise no span is created - as now. It is the responsibility of any proxied services to propagate the x-trace-id to ensure that decision is passed to downstream services.
Zipkin - this tracer propagates its own sampling decision via the X-B3-Sampled header. If that header is not present, such as when creating the root span or it hasn't been propagated with the trace context, then the proxy's tracing (sampling) decision will be used to set the X-B3-Sampled header which will subsequently be passed to downstream services.
Other OpenTracing compatible tracers - the only way to influence the sampling of a trace instance in the OpenTracing standard is through the use of the sampling.priority tag - so in this situation if the proxy sampling decision is false, the sampling.priority tag will be set to 0 when creating the span. It is then the tracer's responsibility to use or ignore that hint.

Risk Level: Medium

Testing: unit tests

Docs Changes:
none

Release Notes:
This change delegates the sampling decision to the tracer, which may have additional context that needs to be taken into account when determining the actual sampling state. If no such state already exists for a trace instance, then the sampling decision supplied by the Envoy proxy will be used.

mattklein123 · 2018-03-16T19:59:02Z

Per new release note policy please add a release note with any applicable feature docs to https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst. Thank you!

mattklein123 · 2018-03-17T19:36:49Z

@objectiser is this still WIP or should this get reviewed?

objectiser · 2018-03-17T20:41:35Z

@mattklein123 It should be ok to review now thanks.

objectiser · 2018-03-17T20:52:21Z

@rnburn Would you be able to have a look at this PR, as it impacts the LightStep and OpenTracing drivers?

codefromthecrypt · 2018-03-19T02:01:21Z

source/common/tracing/zipkin/span_context.h

@@ -17,18 +18,20 @@ class SpanContext {
  /**
   * Default constructor. Creates an empty context.


if empty, please default to false if you must default. I'd hate to see trace id 0 accidentally sent to a zipkin endpoint due to a code error. False already says "don't send this"

codefromthecrypt · 2018-03-19T02:02:42Z

source/common/tracing/zipkin/span_context.h

+  bool sampled() const { return sampled_; }
+
+  /**
+   * @return the sampled flag as a 1-character string.


style choice, but the docs here make me feel like using a literal somehow and then just proving below with a unit test. I don't know if there is an ability to use a literal '1' and '0' in cpp, so if that's not possible ignore me.

codefromthecrypt · 2018-03-19T02:03:09Z

source/common/tracing/zipkin/zipkin_core_types.h

@@ -302,7 +302,8 @@ class Span : public ZipkinBase {
   * Default constructor. Creates an empty span.
   */
  Span()
-      : trace_id_(0), name_(), id_(0), debug_(false), monotonic_start_time_(0), tracer_(nullptr) {}
+      : trace_id_(0), name_(), id_(0), debug_(false), sampled_(true), monotonic_start_time_(0),


again default to sampled false on dummy data

codefromthecrypt · 2018-03-19T02:04:41Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc

+ * Note that a function is needed because the string used to build the regex
+ * cannot be initialized statically.
+ */
+static const std::regex& notSampledRegex() {


most don't use regex, rather '1' or 'true' literal comparison, anything else false (if present). regex tends to be a lot more expensive and warranted if there's a complex decision. If there's secondary reasons for regex, ignoreme

yeah this has already been changed in the main PR

codefromthecrypt · 2018-03-19T02:06:50Z

test/common/tracing/zipkin/zipkin_tracer_impl_test.cc

+
+  // Only context header set is B3 sampled to indicate trace should not be sampled (using legacy
+  // 'false' value)
+  const std::string notSampled = "false";


invert this condition. more important to test 'true' than 'false'

added another test for 'true' case

codefromthecrypt · 2018-03-19T02:08:43Z

test/common/tracing/zipkin/zipkin_tracer_impl_test.cc

+  EXPECT_TRUE(zipkin_span->span().sampled());
+}
+
+TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) {


should a test class include one for absence of XB3Sampled from inbound headers? (I don't see it but might be blind)

The following test does - the trace and span id are set, but the sampling decision is taken from envoy. I will add a nullptr check to make it clearer.

…#2787 Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-03-19T20:08:23Z

@mattklein123 Rebased on the latest master - not sure if this one could make it into 1.6.0 aswell? If possible, then I'll create the release note.

mattklein123 · 2018-03-19T20:09:33Z

@objectiser yeah if we think this is ready I can go through later today. Please do the release note PR. Thanks!

rnburn · 2018-03-19T21:16:12Z

Hey @objectiser, I'm going to add support for sampling.priority to lighstep's tracer. I think things would be a little cleaner if we only used that to specify sampling so that we don't need a special case for the LightStep tracer.

I'll also add constants for the standard tags to OT-C ++ so that they don't need to be hard-coded (opentracing/opentracing-cpp#38).

objectiser · 2018-03-19T22:37:00Z

@rnburn That sounds good in the longer term, but if Matt wants to get this change into the 1.6.0 release, then it would be better to merge the current PR and then simplify it once LightStep supports the tag - as it would be a localised change?

mattklein123 · 2018-03-20T00:04:59Z

@objectiser @rnburn I think there is no urgency to get this into 1.6.0. If there is a cleaner way of doing this it would be great if the two if you can work together to make it happen. Thank you!

objectiser · 2018-03-20T09:36:51Z

@mattklein123 Although not urgent, I think the current way of working will lead to problems when users start using sampling with services that are also instrumented with zipkin compatible tracer. For example, if the inbound proxy decides not to sample, currently it will not create a span and therefore there will be no B3 sampled header passed to the service - and therefore the service's tracer may decide to start a trace.

@rnburn's suggestion essentially means that these lines will be removed (along with the useTagForSamplingDecision method). This could be achieved in a separate PR once the new version of the LightStep tracer is ready.

@rnburn When would the updated LightStep tracer and OT C++ be available?

rnburn · 2018-03-20T13:18:29Z

@objectiser - I can get the updates in today.

But I can also put the changes them in a later PR, if you'd prefer to go ahead with this as is.

objectiser · 2018-03-20T15:24:38Z

@rnburn If you could get the changes in today that would be great.

mandarjog · 2018-03-20T16:36:19Z

@objectiser does this mean http_conn_manager no longer decides what to trace? Will this affect the sampling percentage settings that were implemented there or are all tracers expected to implement them?

objectiser · 2018-03-20T17:07:00Z

@mandarjog no, if the sampling decision for a particular trace instance has not yet been made, then it will use the decision determined by the http_conn_manager.

This PR is about ensuring that the sampling decision propagated to downstream service's via the tracer specific context (e.g. x-b3-sampled header) is not inadvertantly overridden by the downstream envoy proxy just because the service did not also forward the x-trace-id header - which may happen if a service uses a zipkin compatible tracer that handles the trace context propagation, but does not know about the x-tracer-id header.

The other issue as outlined previously is if the envoy proxy sampling decision is false, but the service being proxied also uses a zipkin compatible tracer, then the service's tracer may make its own sampling decision as no x-b3-sampled header was passed in.

mattklein123 · 2018-03-20T17:21:25Z

@objectiser @rnburn sorry, what's the TL/DR on this? Are we going to wait for a parallel @rnburn change to land? If we can have the good solution happen within a week or so should we just wait for that?

objectiser · 2018-03-20T17:42:32Z

@mattklein123 Sounds like @rnburn could have his changes today/tomorrow - however they won't make a big difference to this PR, so would suggest reviewing this PR as is and once ready we either:

(i) merge and do the LightStep related changes in a separate PR
(ii) if the LightStep / OT C++ changes are ready in time, then incorporate those

…ould override sampling Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-03-26T13:31:05Z

Hi @mattklein123 would it be possible to review this PR soon and then when the lightstep changes are available we'll do a separate PR?

mattklein123 · 2018-03-26T16:35:37Z

@objectiser yes I will review today.

mattklein123

Generally LGTM, some small comments. @rnburn can you review also? Thank you.

mattklein123 · 2018-03-26T17:26:34Z

include/envoy/tracing/http_tracer.h

@@ -13,6 +13,19 @@ namespace Tracing {

 enum class OperationName { Ingress, Egress };

+enum class Reason {


I realize that you just moved this code, but can you do us the favor of documenting each of these enum values now that it's in public interface code and other tracers might need to implement?

mattklein123 · 2018-03-26T17:27:33Z

include/envoy/tracing/http_tracer.h

+  ClientForced,
+};
+
+struct Decision {


Same here, can we add some docs?

mattklein123 · 2018-03-26T17:28:22Z

include/envoy/tracing/http_tracer.h

+
+struct Decision {
+  Reason reason;
+  bool is_tracing;


Not your code, but can we rename to "traced" potentially? (Or "perform_trace" or something like that)

mattklein123 · 2018-03-26T17:28:53Z

include/envoy/tracing/http_tracer.h

@@ -88,7 +101,8 @@ class Driver {
   * Start driver specific span.
   */
  virtual SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                            const std::string& operation_name, SystemTime start_time) PURE;
+                            const std::string& operation_name, SystemTime start_time,
+                            const Tracing::Decision& tracing_decision) PURE;


I would pass Tracing::Decision by value as it's a small object.

mattklein123 · 2018-03-26T18:20:38Z

source/common/http/conn_manager_impl.cc

+  active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_,
+                                                       tracing_decision);
+
+  if (!active_span_) {


I can't remember now, but when does this return nullptr? I thought we always have a tracer and this will always return a span (even if the null span).

No, current behaviour at start of traceRequest() is to return if not being sampled, before the span is created. But this check will be removed when lightstep supports the sampling tag.

Ah ok. Got it. Thanks.

mattklein123 · 2018-03-26T18:21:39Z

source/common/tracing/opentracing_driver_impl.cc

+  // If tracing decision is no, and sampling decision is not communicated via tags, then
+  // return a null span to indicate that tracing is not being performed.
+  if (!tracing_decision.is_tracing && !useTagForSamplingDecision()) {
+    return nullptr;


I guess this is related to my other comment. Should we return NullSpan here? I'm not sure if there are other points in the code that might be assuming span is never nullptr? Can you check?

This is just replicating the current nullptr span if is_tracing is false. But this code will be removed once lightstep supports the sampling tag.

Yes, sorry for the delay. I put in lightstep/lightstep-tracer-cpp#94, but there were some concerns around how to handle the propagation of spans with no sampling for lightstep.

I'll follow up with a separate PR once it gets sorted out.

mattklein123 · 2018-03-26T18:22:13Z

source/common/tracing/opentracing_driver_impl.cc

@@ -164,8 +170,13 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea
      tracerStats().span_context_extraction_error_.inc();
    }
  }
-  active_span = tracer.StartSpan(operation_name, {opentracing::ChildOf(parent_span_ctx.get()),
-                                                  opentracing::StartTimestamp(start_time)});
+  opentracing::StartSpanOptions options;


@rnburn Can you review this part?

…Decision by value, changed is_tracing to traced Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123

@objectiser LGTM, thanks. I would like to wait for a @rnburn review before merging. Hopefully he can take a look sometime soon.

rnburn · 2018-03-26T19:49:21Z

source/common/tracing/opentracing_driver_impl.cc

-                                                  opentracing::StartTimestamp(start_time)});
+  opentracing::StartSpanOptions options;
+  opentracing::ChildOf(parent_span_ctx.get()).Apply(options);
+  opentracing::StartTimestamp(start_time).Apply(options);


This will work; though it would be more direct to write

options.references.emplace_back(opentracing::SpanReferenceType::ChildOfRef, parent_span_context.get()); options.start_system_timestamp = start_time;

Those objects are really just to make specifying options with the StartSpan method convenient.

Had trouble making this change - compilation errors. Possibly this could be done as part of your PR?

I made a typo (corrected now)

options.references.emplace_back(opentracing::SpanReferenceType::ChildOf, parent_span_context.get());

should have been

options.references.emplace_back(opentracing::SpanReferenceType::ChildOfRef, parent_span_context.get());

I can verify the code tomorrow.

rnburn · 2018-03-26T19:53:16Z

source/common/tracing/opentracing_driver_impl.cc

+  opentracing::ChildOf(parent_span_ctx.get()).Apply(options);
+  opentracing::StartTimestamp(start_time).Apply(options);
+  if (!tracing_decision.traced) {
+    opentracing::SetTag(OpenTracingTags::get().SAMPLING_PRIORITY, 0).Apply(options);


I added a constant for sampling priority to match what's in the other APIs so you could write

options.tags.emplace_back(opentracing::ext::sampling_priority, 0);

Updated to use the new constant.

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-03-27T09:21:44Z

@rnburn Thanks - that worked. However seems to be a build problem on mac with the newer version of opentracing?

rnburn · 2018-03-27T14:10:58Z

@objectiser - I put in a small change to the bazel build in opentracing/opentracing-cpp#69. It should work on OSX now.

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 · 2018-03-27T15:59:20Z

@rnburn @objectiser I'm not totally following the back and forth. Is this ready? @rnburn do you approve?

rnburn · 2018-03-27T16:01:19Z

Yes, it looks good to me. 👍

objectiser · 2018-03-27T16:24:33Z

@rnburn Thanks for the quick fix.

@mattklein123 Hopefully good to go now.

mattklein123

Thank you @objectiser and @rnburn!

objectiser force-pushed the envoysampling branch 3 times, most recently from 24d43fe to a53fb24 Compare March 13, 2018 15:49

objectiser changed the title ~~WIP tracing: Delegate envoy tracing decision to tracer to determine if should override sampling~~ tracing: Delegate envoy tracing decision to tracer to determine if should override sampling Mar 17, 2018

objectiser mentioned this pull request Mar 17, 2018

tracing: Implement propagation and use of the X-B3-Sampled header. #2764

Merged

codefromthecrypt reviewed Mar 19, 2018

View reviewed changes

objectiser added a commit to objectiser/envoy that referenced this pull request Mar 19, 2018

Updates based on additional comments from adriancole on PR envoyproxy…

7299c90

…#2787 Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the envoysampling branch from a53fb24 to 8d1454e Compare March 19, 2018 19:24

mattklein123 self-assigned this Mar 19, 2018

objectiser mentioned this pull request Mar 19, 2018

Release note related to envoy #2787 envoyproxy/data-plane-api#567

Merged

rnburn mentioned this pull request Mar 20, 2018

Add support for sampling.priority. lightstep/lightstep-tracer-cpp#94

Merged

tracing: Delegate envoy tracing decision to tracer to determine if sh…

4160ed8

…ould override sampling Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the envoysampling branch from 8d1454e to 4160ed8 Compare March 26, 2018 13:10

mattklein123 reviewed Mar 26, 2018

View reviewed changes

Updates based on comments, added docs to Reason and Decision, passed …

682be76

…Decision by value, changed is_tracing to traced Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 reviewed Mar 26, 2018

View reviewed changes

rnburn reviewed Mar 26, 2018

View reviewed changes

objectiser added 3 commits March 26, 2018 23:32

Use the OpenTracing CPP standard tag for sample.priority

9db12fa

Signed-off-by: Gary Brown <gary@brownuk.com>

Fix format

e08590d

Signed-off-by: Gary Brown <gary@brownuk.com>

Updated based on rnburn's suggestions

2dcda81

Signed-off-by: Gary Brown <gary@brownuk.com>

Update to latest opentracing-cpp to pickup mac build fix

0090e54

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 approved these changes Mar 27, 2018

View reviewed changes

mattklein123 merged commit 85fe233 into envoyproxy:master Mar 27, 2018

objectiser deleted the envoysampling branch March 27, 2018 16:41

phylake mentioned this pull request Jul 28, 2020

Dynamic OT tracing question #12291

Closed

		@@ -17,18 +18,20 @@ class SpanContext {
		/**
		* Default constructor. Creates an empty context.

		@@ -13,6 +13,19 @@ namespace Tracing {

		enum class OperationName { Ingress, Egress };

		enum class Reason {

tracing: Delegate envoy tracing decision to tracer to determine if should override sampling #2787

tracing: Delegate envoy tracing decision to tracer to determine if should override sampling #2787

Conversation

objectiser commented Mar 13, 2018 • edited Loading

mattklein123 commented Mar 16, 2018

mattklein123 commented Mar 17, 2018

objectiser commented Mar 17, 2018

objectiser commented Mar 17, 2018

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

Choose a reason for hiding this comment

objectiser Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

objectiser commented Mar 19, 2018 • edited Loading

mattklein123 commented Mar 19, 2018

rnburn commented Mar 19, 2018

objectiser commented Mar 19, 2018

mattklein123 commented Mar 20, 2018

objectiser commented Mar 20, 2018

rnburn commented Mar 20, 2018

objectiser commented Mar 20, 2018

mandarjog commented Mar 20, 2018

objectiser commented Mar 20, 2018 • edited Loading

mattklein123 commented Mar 20, 2018

objectiser commented Mar 20, 2018

objectiser commented Mar 26, 2018 • edited Loading

mattklein123 commented Mar 26, 2018

mattklein123 left a 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

objectiser Mar 26, 2018 • 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

mattklein123 left a comment

Choose a reason for hiding this comment

rnburn Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

rnburn Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Mar 27, 2018

rnburn commented Mar 27, 2018

mattklein123 commented Mar 27, 2018

rnburn commented Mar 27, 2018

objectiser commented Mar 27, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

objectiser commented Mar 13, 2018 •

edited

Loading

objectiser Mar 19, 2018 •

edited

Loading

objectiser commented Mar 19, 2018 •

edited

Loading

objectiser commented Mar 20, 2018 •

edited

Loading

objectiser commented Mar 26, 2018 •

edited

Loading

objectiser Mar 26, 2018 •

edited

Loading

rnburn Mar 26, 2018 •

edited

Loading

rnburn Mar 27, 2018 •

edited

Loading

rnburn Mar 26, 2018 •

edited

Loading