tracing: add SpanFinalizer and test and use NullSpan #1029

goaway · 2017-05-30T17:06:42Z

No description provided.

RomanDzhabarov · 2017-05-30T18:57:02Z

source/common/http/conn_manager_impl.cc

+  if (request_info_.healthCheck()) {
+    connection_manager_.config_.tracingStats().health_check_.inc();
+  } else {
+    Tracing::DefaultIngressFinalizer&& finalizer{*request_headers_, request_info_, *this};


why do we need && here?

as for the name, maybe, Tracing::DefaultConnManagerFinalizer ?

RomanDzhabarov · 2017-05-30T20:01:28Z

include/envoy/tracing/http_tracer.h

 class Span;

 typedef std::unique_ptr<Span> SpanPtr;

+class SpanFinalizer;


you can just do full class declaration here and move everything from https://github.com/lyft/envoy/pull/1029/files#diff-7b5f0d3c634c8c1ac2d4fcaa26541dd9R79

RomanDzhabarov · 2017-05-30T20:02:11Z

include/envoy/tracing/http_tracer.h

@@ -52,8 +54,9 @@ class Span {
  /**
   * Capture the final duration for this Span and carry out any work necessary to complete it.
   * Once this method is called, the Span may be safely discarded.
+   * @param finalizer may be provided to perform context-specific work to complete the Span


shall we get rid of "may be"?

RomanDzhabarov · 2017-05-30T20:03:01Z

source/common/tracing/http_tracer_impl.cc

-void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
-                                     const Http::AccessLog::RequestInfo& request_info,
-                                     const Config& config) {
+DefaultIngressFinalizer::DefaultIngressFinalizer(Http::HeaderMap& request_headers,


DefaultConnManagerFinalizer ?

RomanDzhabarov · 2017-05-30T20:04:58Z

test/common/tracing/lightstep_tracer_impl_test.cc

@@ -166,10 +166,16 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) {
      .WillOnce(Return(5000U));

  SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  first_span->finishSpan();
+  // MockFinalizer finalizer{};
+  NullFinalizer finalizer{};


NullFinalizer finalizer; // {} is redundant

RomanDzhabarov · 2017-05-31T00:36:28Z

test/common/tracing/lightstep_tracer_impl_test.cc

@@ -166,10 +166,16 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) {
      .WillOnce(Return(5000U));

  SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  first_span->finishSpan();
+  // MockFinalizer finalizer;
+  NullFinalizer finalizer;


why not MockFinalizer, we could check later on that finalize is called on this mock

RomanDzhabarov · 2017-05-31T00:36:50Z

test/common/tracing/zipkin/zipkin_tracer_impl_test.cc

@@ -153,10 +153,16 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpans) {
      .WillOnce(Return(5000U));

  Tracing::SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  first_span->finishSpan();
+  // Tracing::MockFinalizer finalizer;


same here with MockFinalizer

RomanDzhabarov · 2017-05-31T00:37:07Z

LGTM, few nits.

RomanDzhabarov · 2017-05-31T00:37:27Z

@htuch do you want to take a pass on this PR

htuch · 2017-05-31T18:27:48Z

@goaway The ASAN failures look legit, do you want to take a look?

htuch

The changes look good, I'm wondering if you could add a commit comment for the benefit of myself and others who are unfamiliar with the tracing internals regarding motivation. I.e. what future code changes is this preparing for or helping with? It seems the main advantage it confers is allowing the null span to be treated more uniformally, but I'm probably missing something.

htuch · 2017-05-31T18:31:29Z

source/common/http/conn_manager_impl.cc

+  if (request_info_.healthCheck()) {
+    connection_manager_.config_.tracingStats().health_check_.inc();
+  } else {
+    Tracing::HttpConnManFinalizerImpl finalizer{*request_headers_, request_info_, *this};


Is it worth constructing the finalizer here in the common case (e.g. when not tracing) that active_span_ is NullSpan?

Possibly not, especially given it's not all that cheap to construct. I can't think of a super clean way to check that here though. I mean we could have an isNull check on the Span itself, though that doesn't seem all that great. Any suggestions?

I think we all agree that this is not necessary, but it involves 3 ref copies during the creation ultimately we could fix with isNull check if this appears in the perf traces.
Open to suggestions as well

If this is the only place we do this we could consider just keeping track of the tracing decision from where active_span_ was set, but then you're essentially going back to treating this as nullptr. In general though it seems that there is the pattern of "some setup work; do something on a span that might be null;", so it would be useful to have a cheap way to know if actively tracing. iSNull seems reasonable in the absence of some other object with trace context info.

I think @RomanDzhabarov has a fair point - all the really expensive stuff is in the actual finalize() call, which gets skipped entirely in the case of a NullSpan (I for some reason was thinking for a moment that more of it was in the constructor, but now recall I deliberately pushed it all into the finalize call for that reason).

I'm leaning slightly towards sticking with the unnecessary instantiation for now, since it keeps things a little tidier. No very strong feelings about it though.

OK, it's fine for now, we can solve this issue further down the track if/when the unnecessary work ends up being larger.

goaway · 2017-06-01T17:58:57Z

In addition to the above, it also supports upcoming work to utilize Spans in other contexts (particularly in AsyncClient). The logic in the static utility currently used to wrap up the span is now bundled into a SpanFinalizer, and other places can define their own SpanFinalizers without complicating the interface of Span itself.

The idea is to be able to pass Spans around, and spawn child Spans off of them, without needing to maintain references back to original Tracers or context of the parent.

htuch · 2017-06-01T18:28:13Z

Makes sense, thanks, merger should squash that into the commit message when ready to ship.

htuch · 2017-06-01T22:09:28Z

@goaway LGTM, but ASAN/TSAN are still failing, can you take a look at them? Thanks.

RomanDzhabarov · 2017-06-05T21:34:27Z

while Mike is OOO until June 15th, I'm going to close this one for now.

mattklein123 · 2017-06-09T00:14:51Z

@RomanDzhabarov @goaway I pushed fix for asan/tsan issue. #winner

RomanDzhabarov

yay

mattklein123 · 2017-06-09T00:18:22Z

I will review tomorrow morning completely. Then we can merge.

mattklein123 · 2017-06-09T15:15:38Z

This is still failing some tests. Sigh. I will deal with this today.

mattklein123 · 2017-06-09T18:30:04Z

@RomanDzhabarov the remaining issue is that in conn manager, request headers might be null when the stream is destroyed. We need to handle that case. Can you look into/fix.

RomanDzhabarov · 2017-06-09T18:31:01Z

Yup, i'll push update today

RomanDzhabarov · 2017-06-09T20:46:50Z

@mattklein123 all good now.

mattklein123

yup nice. looks good.

…tions (#1029) Description: Introduces platform-bridged filter interfaces that allow stopping and stopping filter iteration, and asynchronous resumption of filter chain. The following conventions are adopted with these new interfaces: - 'Continue' is the status used to indicate HTTP entities should be forwarded when filter iteration is ongoing. - 'StopIteration' causes forwarding to halt (though invocations of the current filter will continue). - The 'ResumeIteration' status or an asynchronous resume call must be used to begin iteration and forwarding again. 'Continue' is not valid when iteration is stopped. - Using an asynchronous call to resume iteration will result in a special 'onResume' invocation of the filter, that will allow interaction with the HTTP stream on the same thread as other invocations, avoiding the need for synchronization. - Resume mechanisms offer optional parameters attached to the return status that can be used to provide parts of the stream that have not yet been forwarded. e.g., if you resume during an 'onData' invocation, you can attach headers to the 'ResumeIteration' status (and must do so, if headers have not yet been forwarded). Risk: Low Testing: Local and CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

goaway added 3 commits May 26, 2017 10:40

tracing: adopt NullSpan and add finalizer

8e59030

checkpoint: compiles

3e83c33

checkpoint: tests pass

4ea0ef7

RomanDzhabarov self-assigned this May 30, 2017

RomanDzhabarov reviewed May 30, 2017

View reviewed changes

minor fix

53ec11b

RomanDzhabarov reviewed May 30, 2017

View reviewed changes

goaway added 3 commits May 30, 2017 15:22

updates for comments

1ffcde9

improve test

0956877

delete stray comment

8fc3d01

RomanDzhabarov reviewed May 31, 2017

View reviewed changes

htuch reviewed May 31, 2017

View reviewed changes

update tests

a67ef7c

asan and tsan failurse

469f3c3

RomanDzhabarov closed this Jun 5, 2017

mattklein123 mentioned this pull request Jun 8, 2017

Caught Segmentation fault #1064

Closed

RomanDzhabarov reopened this Jun 8, 2017

RomanDzhabarov added 2 commits June 8, 2017 12:38

remove comment.

a70d244

Merge branch 'master' into nullspan

5d7d560

fix

27b0875

RomanDzhabarov previously approved these changes Jun 9, 2017

View reviewed changes

htuch previously approved these changes Jun 9, 2017

View reviewed changes

pass header map *

5b91a78

RomanDzhabarov dismissed stale reviews from htuch and themself via 5b91a78 June 9, 2017 19:56

mattklein123 approved these changes Jun 9, 2017

View reviewed changes

RomanDzhabarov merged commit ddf9cc9 into master Jun 9, 2017

RomanDzhabarov deleted the nullspan branch June 12, 2017 19:34

mattklein123 unassigned RomanDzhabarov Sep 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: add SpanFinalizer and test and use NullSpan #1029

tracing: add SpanFinalizer and test and use NullSpan #1029

goaway commented May 30, 2017

RomanDzhabarov May 30, 2017

RomanDzhabarov May 30, 2017

RomanDzhabarov May 30, 2017

RomanDzhabarov May 30, 2017

RomanDzhabarov May 30, 2017

RomanDzhabarov May 31, 2017

RomanDzhabarov May 31, 2017

RomanDzhabarov commented May 31, 2017

RomanDzhabarov commented May 31, 2017

htuch commented May 31, 2017

htuch left a comment

htuch May 31, 2017

goaway Jun 1, 2017

RomanDzhabarov Jun 1, 2017

htuch Jun 1, 2017

goaway Jun 1, 2017

htuch Jun 1, 2017

goaway commented Jun 1, 2017

htuch commented Jun 1, 2017

htuch commented Jun 1, 2017

RomanDzhabarov commented Jun 5, 2017

mattklein123 commented Jun 9, 2017

RomanDzhabarov left a comment

mattklein123 commented Jun 9, 2017

mattklein123 commented Jun 9, 2017

mattklein123 commented Jun 9, 2017

RomanDzhabarov commented Jun 9, 2017

RomanDzhabarov commented Jun 9, 2017

mattklein123 left a comment

tracing: add SpanFinalizer and test and use NullSpan #1029

tracing: add SpanFinalizer and test and use NullSpan #1029

Conversation

goaway commented May 30, 2017

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

RomanDzhabarov commented May 31, 2017

RomanDzhabarov commented May 31, 2017

htuch commented May 31, 2017

htuch 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

Choose a reason for hiding this comment

goaway commented Jun 1, 2017

htuch commented Jun 1, 2017

htuch commented Jun 1, 2017

RomanDzhabarov commented Jun 5, 2017

mattklein123 commented Jun 9, 2017

RomanDzhabarov left a comment

Choose a reason for hiding this comment

mattklein123 commented Jun 9, 2017

mattklein123 commented Jun 9, 2017

mattklein123 commented Jun 9, 2017

RomanDzhabarov commented Jun 9, 2017

RomanDzhabarov commented Jun 9, 2017

mattklein123 left a comment

Choose a reason for hiding this comment