tracing: Zipkin: do cleanup on tracer destruction in order to support dynamic (re-)configuration#10349
Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom Mar 24, 2020
Conversation
yskopets
force-pushed
the
feature/do-cleanup-on-zipkin-tracer-destruction
branch
4 times, most recently
from
138d721 to
dc90426
Compare
Member
Author
|
@alyssawilk @jmarantz I will split this PR into 2:
|
yskopets
force-pushed
the
feature/do-cleanup-on-zipkin-tracer-destruction
branch
4 times, most recently
from
2c2f0e4 to
7900a30
Compare
yskopets
requested review from
alyssawilk,
dio,
lizan and
mattklein123
as code owners
Member
Author
yskopets
force-pushed
the
feature/do-cleanup-on-zipkin-tracer-destruction
branch
4 times, most recently
from
317a9e6 to
11ff67f
Compare
yskopets
force-pushed
the
feature/do-cleanup-on-zipkin-tracer-destruction
branch
2 times, most recently
from
790b7ca to
0563154
Compare
Contributor
|
Clarification: this PR is no longer intended for review, is that right? /wait |
Member
Author
… dynamic (re-)configuration Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
yskopets
force-pushed
the
feature/do-cleanup-on-zipkin-tracer-destruction
branch
from
0563154 to
aaa4aa5
Compare
Member
Author
|
@jmarantz It is now ready for review. All dependant PRs have been merged. |
jmarantz
reviewed
Mar 24, 2020
| .httpAsyncClientForCluster(driver_.cluster()->name()) | ||
| .send(std::move(message), *this, | ||
| Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(timeout))); | ||
| auto* request = driver_.clusterManager() |
Contributor
There was a problem hiding this comment.
please use an explicit type here.
| const CollectorInfo collector_; | ||
| SpanBufferPtr span_buffer_; | ||
| // Track active HTTP requests to be able to cancel them on destruction. | ||
| Http::AsyncClientRequestTracker active_requests_; |
Contributor
There was a problem hiding this comment.
qq: is this class instantiated per-thread? Or if it's shared do we need to guard with a mutex?
Member
Author
There was a problem hiding this comment.
yes, separate instance per thread
tls_->set([this, collector, &random_generator, trace_id_128bit, shared_span_context](
Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr {
...
ReporterImpl::NewInstance(std::ref(*this), std::ref(dispatcher), collector));
...
});
Contributor
|
/wait |
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
jmarantz
approved these changes
Mar 24, 2020
Contributor
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Thanks!
@envoyproxy/senior-maintainers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description: do cleanup on tracer destruction in order to support dynamic (re-)configuration
Risk Level: Low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
Context:
depends on async client: refactorHttp::AsyncClient::Callbacksto pass inHttp::AsyncClient::Request*for correlation between requests and responses #10358 and async client: introduceAsyncClientRequestTrackerto keep track of inflight requests and cancel them on destruction #10359Motivation: