-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tracing: add SpanFinalizer and test and use NullSpan #1029
Conversation
if (request_info_.healthCheck()) { | ||
connection_manager_.config_.tracingStats().health_check_.inc(); | ||
} else { | ||
Tracing::DefaultIngressFinalizer&& finalizer{*request_headers_, request_info_, *this}; |
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.
why do we need && here?
as for the name, maybe, Tracing::DefaultConnManagerFinalizer ?
include/envoy/tracing/http_tracer.h
Outdated
class Span; | ||
|
||
typedef std::unique_ptr<Span> SpanPtr; | ||
|
||
class SpanFinalizer; |
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.
you can just do full class declaration here and move everything from https://github.com/lyft/envoy/pull/1029/files#diff-7b5f0d3c634c8c1ac2d4fcaa26541dd9R79
include/envoy/tracing/http_tracer.h
Outdated
@@ -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 |
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.
shall we get rid of "may be"?
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, |
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.
DefaultConnManagerFinalizer ?
@@ -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{}; |
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.
NullFinalizer finalizer; // {} is redundant
@@ -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; |
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.
why not MockFinalizer, we could check later on that finalize is called on this mock
@@ -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; |
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.
same here with MockFinalizer
LGTM, few nits. |
@htuch do you want to take a pass on this PR |
@goaway The ASAN failures look legit, do you want to take a look? |
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.
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.
if (request_info_.healthCheck()) { | ||
connection_manager_.config_.tracingStats().health_check_.inc(); | ||
} else { | ||
Tracing::HttpConnManFinalizerImpl finalizer{*request_headers_, request_info_, *this}; |
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.
Is it worth constructing the finalizer here in the common case (e.g. when not tracing) that active_span_
is NullSpan
?
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.
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?
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.
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
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.
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.
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.
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.
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.
OK, it's fine for now, we can solve this issue further down the track if/when the unnecessary work ends up being larger.
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. |
Makes sense, thanks, merger should squash that into the commit message when ready to ship. |
@goaway LGTM, but ASAN/TSAN are still failing, can you take a look at them? Thanks. |
while Mike is OOO until June 15th, I'm going to close this one for now. |
@RomanDzhabarov @goaway I pushed fix for asan/tsan issue. #winner |
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.
yay
I will review tomorrow morning completely. Then we can merge. |
This is still failing some tests. Sigh. I will deal with this today. |
@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. |
Yup, i'll push update today |
@mattklein123 all good now. |
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.
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>
…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>
No description provided.