From ee1263fb978c1d241ad31b9e8614b6e7dddaeafc Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Tue, 4 Oct 2016 20:22:58 -0700 Subject: [PATCH 1/6] Flush spans on timer. --- source/common/tracing/http_tracer_impl.cc | 47 +++++++++++++++-------- source/common/tracing/http_tracer_impl.h | 16 +++++--- source/server/configuration_impl.cc | 2 +- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 2626753cb434..1677469b8607 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -115,7 +115,13 @@ void HttpTracerImpl::populateStats(const Decision& decision) { } LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink) - : builder_(tracer), sink_(sink) {} + : builder_(tracer), sink_(sink) { + flush_timer_ = sink_.dispatcher().createTimer([this]() -> void { + sink_.tracerStats().spans_flushed_.inc(); + flushSpans(); + }); + flush_timer_->enableTimer(std::chrono::milliseconds(1000)); +} void LightStepRecorder::RecordSpan(lightstep::collector::Span&& span) { builder_.addSpan(std::move(span)); @@ -123,19 +129,7 @@ void LightStepRecorder::RecordSpan(lightstep::collector::Span&& span) { uint64_t min_flush_spans = sink_.runtime().snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); if (builder_.pendingSpans() == min_flush_spans) { - sink_.tracerStats().spans_sent_.add(min_flush_spans); - lightstep::collector::ReportRequest request; - std::swap(request, builder_.pending()); - - Http::MessagePtr message = - Grpc::Common::prepareHeaders(sink_.collectorCluster(), LightStepSink::LIGHTSTEP_SERVICE, - LightStepSink::LIGHTSTEP_METHOD); - - message->body(Grpc::Common::serializeBody(std::move(request))); - - sink_.clusterManager() - .httpAsyncClientForCluster(sink_.collectorCluster()) - .send(std::move(message), *this, std::chrono::milliseconds(5000)); + flushSpans(); } } @@ -150,12 +144,31 @@ LightStepRecorder::NewInstance(LightStepSink& sink, const lightstep::TracerImpl& return std::unique_ptr(new LightStepRecorder(tracer, sink)); } +void LightStepRecorder::flushSpans() { + sink_.tracerStats().spans_sent_.add(builder_.pendingSpans()); + lightstep::collector::ReportRequest request; + std::swap(request, builder_.pending()); + + Http::MessagePtr message = Grpc::Common::prepareHeaders( + sink_.collectorCluster(), LightStepSink::LIGHTSTEP_SERVICE, LightStepSink::LIGHTSTEP_METHOD); + + message->body(Grpc::Common::serializeBody(std::move(request))); + + sink_.clusterManager() + .httpAsyncClientForCluster(sink_.collectorCluster()) + .send(std::move(message), *this, std::chrono::milliseconds(5000)); +} + +LightStepSink::TlsLightStepTracer::TlsLightStepTracer(lightstep::Tracer tracer, LightStepSink& sink) + : tracer_(tracer), sink_(sink) {} + LightStepSink::LightStepSink(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Stats::Store& stats, const std::string& service_node, - ThreadLocal::Instance& tls, Runtime::Loader& runtime, + Event::Dispatcher& dispatcher, Stats::Store& stats, + const std::string& service_node, ThreadLocal::Instance& tls, + Runtime::Loader& runtime, std::unique_ptr options) : collector_cluster_(config.getString("collector_cluster")), cm_(cluster_manager), - stats_store_(stats), + dispatcher_(dispatcher), stats_store_(stats), tracer_stats_{LIGHTSTEP_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, service_node_(service_node), tls_(tls), runtime_(runtime), options_(std::move(options)), tls_slot_(tls.allocateSlot()) { diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index b34a0e0cb5bd..9c5a81ea1efc 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -28,7 +28,9 @@ struct HttpTracerStats { HTTP_TRACER_STATS(GENERATE_COUNTER_STRUCT) }; -#define LIGHTSTEP_TRACER_STATS(COUNTER) COUNTER(spans_sent) +#define LIGHTSTEP_TRACER_STATS(COUNTER) \ + COUNTER(spans_sent) \ + COUNTER(spans_flushed) struct LightstepTracerStats { LIGHTSTEP_TRACER_STATS(GENERATE_COUNTER_STRUCT) @@ -98,8 +100,9 @@ class HttpTracerImpl : public HttpTracer { class LightStepSink : public HttpSink { public: LightStepSink(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Stats::Store& stats, const std::string& service_node, ThreadLocal::Instance& tls, - Runtime::Loader& runtime, std::unique_ptr options); + Event::Dispatcher& dispatcher, Stats::Store& stats, const std::string& service_node, + ThreadLocal::Instance& tls, Runtime::Loader& runtime, + std::unique_ptr options); // Tracer::HttpSink void flushTrace(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, @@ -110,14 +113,14 @@ class LightStepSink : public HttpSink { Runtime::Loader& runtime() { return runtime_; } Stats::Store& statsStore() { return stats_store_; } LightstepTracerStats& tracerStats() { return tracer_stats_; } + Event::Dispatcher& dispatcher() { return dispatcher_; } static const std::string LIGHTSTEP_SERVICE; static const std::string LIGHTSTEP_METHOD; private: struct TlsLightStepTracer : ThreadLocal::ThreadLocalObject { - TlsLightStepTracer(lightstep::Tracer tracer, LightStepSink& sink) - : tracer_(tracer), sink_(sink) {} + TlsLightStepTracer(lightstep::Tracer tracer, LightStepSink& sink); void shutdown() override {} @@ -131,6 +134,7 @@ class LightStepSink : public HttpSink { const std::string collector_cluster_; Upstream::ClusterManager& cm_; + Event::Dispatcher& dispatcher_; Stats::Store& stats_store_; LightstepTracerStats tracer_stats_; const std::string service_node_; @@ -152,12 +156,14 @@ class LightStepRecorder : public lightstep::Recorder, Http::AsyncClient::Callbac void onSuccess(Http::MessagePtr&&) override; void onFailure(Http::AsyncClient::FailureReason) override; + void flushSpans(); static std::unique_ptr NewInstance(LightStepSink& sink, const lightstep::TracerImpl& tracer); private: lightstep::ReportBuilder builder_; LightStepSink& sink_; + Event::TimerPtr flush_timer_; }; } // Tracing diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 89fa9df5a351..48273f7ef069 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -88,7 +88,7 @@ void MainImpl::initializeTracers(const Json::Object& tracing_configuration_) { opts->guid_generator = [&rand]() { return rand.random(); }; http_tracer_->addSink(Tracing::HttpSinkPtr{new Tracing::LightStepSink( - sink.getObject("config"), *cluster_manager_, server_.stats(), + sink.getObject("config"), *cluster_manager_, server_.dispatcher(), server_.stats(), server_.options().serviceNodeName(), server_.threadLocal(), server_.runtime(), std::move(opts))}); } else { From b5c8ad329fa7dffba946fb515097880c65c6f76f Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 5 Oct 2016 11:05:37 -0700 Subject: [PATCH 2/6] Add test. --- source/common/tracing/http_tracer_impl.cc | 1 + test/common/tracing/http_tracer_impl_test.cc | 65 +++++++++++++++----- test/mocks/event/mocks.h | 2 +- 3 files changed, 50 insertions(+), 18 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 1677469b8607..2b72ca2b5a54 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -119,6 +119,7 @@ LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightS flush_timer_ = sink_.dispatcher().createTimer([this]() -> void { sink_.tracerStats().spans_flushed_.inc(); flushSpans(); + flush_timer_->enableTimer(std::chrono::milliseconds(1000)); }); flush_timer_->enableTimer(std::chrono::milliseconds(1000)); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index c7fa378d50ce..6efb19352a70 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -272,14 +272,21 @@ TEST(HttpNullTracerTest, NoFailures) { class LightStepSinkTest : public Test { public: - void setup(Json::Object& config) { + void setup(Json::Object& config, bool init_timer) { std::unique_ptr opts(new lightstep::TracerOptions()); opts->access_token = "sample_token"; - opts->tracer_attributes["lightstep.guid"] = "random_guid"; opts->tracer_attributes["lightstep.component_name"] = "component"; - sink_.reset( - new LightStepSink(config, cm_, stats_, "service_node", tls_, runtime_, std::move(opts))); + ON_CALL(cm_, httpAsyncClientForCluster("lightstep_saas")) + .WillByDefault(ReturnRef(cm_.async_client_)); + + if (init_timer) { + timer_ = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); + } + + sink_.reset(new LightStepSink(config, cm_, dispatcher_, stats_, "service_node", tls_, runtime_, + std::move(opts))); } void setupValidSink() { @@ -291,17 +298,19 @@ class LightStepSinkTest : public Test { )EOF"; Json::StringLoader loader(valid_config); - setup(loader); + setup(loader, true); } const Http::HeaderMapImpl empty_header_{}; + std::unique_ptr sink_; + NiceMock* timer_; Stats::IsolatedStoreImpl stats_; NiceMock cm_; + NiceMock dispatcher_; NiceMock cluster_; NiceMock random_; NiceMock runtime_; - std::unique_ptr sink_; NiceMock tls_; }; @@ -312,14 +321,14 @@ TEST_F(LightStepSinkTest, InitializeSink) { )EOF"; Json::StringLoader loader(invalid_config); - EXPECT_THROW(setup(loader), EnvoyException); + EXPECT_THROW(setup(loader, false), EnvoyException); } { std::string empty_config = "{}"; Json::StringLoader loader(empty_config); - EXPECT_THROW(setup(loader), EnvoyException); + EXPECT_THROW(setup(loader, false), EnvoyException); } { @@ -331,7 +340,7 @@ TEST_F(LightStepSinkTest, InitializeSink) { )EOF"; Json::StringLoader loader(valid_config); - EXPECT_THROW(setup(loader), EnvoyException); + EXPECT_THROW(setup(loader, false), EnvoyException); } { @@ -344,7 +353,7 @@ TEST_F(LightStepSinkTest, InitializeSink) { )EOF"; Json::StringLoader loader(valid_config); - EXPECT_THROW(setup(loader), EnvoyException); + EXPECT_THROW(setup(loader, false), EnvoyException); } { @@ -356,7 +365,7 @@ TEST_F(LightStepSinkTest, InitializeSink) { )EOF"; Json::StringLoader loader(valid_config); - setup(loader); + setup(loader, true); } } @@ -364,9 +373,6 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; - EXPECT_CALL(cm_, httpAsyncClientForCluster("lightstep_saas")) - .WillOnce(ReturnRef(cm_.async_client_)); - Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); @@ -434,13 +440,38 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { EXPECT_EQ(2U, stats_.counter("tracing.lightstep.spans_sent").value()); } -TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { +TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; - EXPECT_CALL(cm_, httpAsyncClientForCluster("lightstep_saas")) - .WillOnce(ReturnRef(cm_.async_client_)); + const Optional timeout(std::chrono::seconds(5)); + EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); + + SystemTime start_time; + EXPECT_CALL(request_info, startTime()).WillOnce(Return(start_time)); + Optional code(200); + EXPECT_CALL(request_info, responseCode()).Times(2).WillRepeatedly(ReturnRef(code)); + + const std::string protocol = "http/1"; + EXPECT_CALL(request_info, protocol()).WillOnce(ReturnRef(protocol)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) + .WillOnce(Return(5)); + + sink_->flushTrace(empty_header_, empty_header_, request_info); + // Timer should be re-enabled. + EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); + + timer_->callback_(); + + EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_flushed").value()); + EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value()); +} + +TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { + setupValidSink(); + + NiceMock request_info; Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index b0ca6e5665df..34215e9300db 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -97,7 +97,7 @@ class MockTimer : public Timer { // Event::Timer MOCK_METHOD0(disableTimer, void()); - MOCK_METHOD1(enableTimer, void(const std::chrono::milliseconds& d)); + MOCK_METHOD1(enableTimer, void(const std::chrono::milliseconds&)); TimerCb callback_; }; From 844db8d5787a928f85505781bbc78e940103888c Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 5 Oct 2016 11:10:05 -0700 Subject: [PATCH 3/6] refine stat name. --- source/common/tracing/http_tracer_impl.cc | 2 +- source/common/tracing/http_tracer_impl.h | 2 +- test/common/tracing/http_tracer_impl_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 2b72ca2b5a54..13dd97b90900 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -117,7 +117,7 @@ void HttpTracerImpl::populateStats(const Decision& decision) { LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink) : builder_(tracer), sink_(sink) { flush_timer_ = sink_.dispatcher().createTimer([this]() -> void { - sink_.tracerStats().spans_flushed_.inc(); + sink_.tracerStats().timer_flushed_.inc(); flushSpans(); flush_timer_->enableTimer(std::chrono::milliseconds(1000)); }); diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 9c5a81ea1efc..7fc94c8229cc 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -30,7 +30,7 @@ struct HttpTracerStats { #define LIGHTSTEP_TRACER_STATS(COUNTER) \ COUNTER(spans_sent) \ - COUNTER(spans_flushed) + COUNTER(timer_flushed) struct LightstepTracerStats { LIGHTSTEP_TRACER_STATS(GENERATE_COUNTER_STRUCT) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 6efb19352a70..84950cc08cbe 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -464,7 +464,7 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { timer_->callback_(); - EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_flushed").value()); + EXPECT_EQ(1U, stats_.counter("tracing.lightstep.timer_flushed").value()); EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value()); } From 4e97b7209501893090be67e70de8a564b29531e0 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 5 Oct 2016 11:55:25 -0700 Subject: [PATCH 4/6] Working on comments, need to fix tests. --- source/common/tracing/http_tracer_impl.cc | 54 ++++++++++++++--------- source/common/tracing/http_tracer_impl.h | 4 +- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 13dd97b90900..686895acce39 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -114,14 +114,20 @@ void HttpTracerImpl::populateStats(const Decision& decision) { } } -LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink) +LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink, + Event::Dispatcher& dispatcher) : builder_(tracer), sink_(sink) { - flush_timer_ = sink_.dispatcher().createTimer([this]() -> void { + flush_timer_ = dispatcher.createTimer([this]() -> void { sink_.tracerStats().timer_flushed_.inc(); flushSpans(); - flush_timer_->enableTimer(std::chrono::milliseconds(1000)); + uint64_t flush_interval = + sink_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); + flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); }); - flush_timer_->enableTimer(std::chrono::milliseconds(1000)); + + uint64_t flush_interval = + sink_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); + flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); } void LightStepRecorder::RecordSpan(lightstep::collector::Span&& span) { @@ -141,23 +147,29 @@ bool LightStepRecorder::FlushWithTimeout(lightstep::Duration) { } std::unique_ptr -LightStepRecorder::NewInstance(LightStepSink& sink, const lightstep::TracerImpl& tracer) { - return std::unique_ptr(new LightStepRecorder(tracer, sink)); +LightStepRecorder::NewInstance(LightStepSink& sink, Event::Dispatcher& dispatcher, + const lightstep::TracerImpl& tracer) { + return std::unique_ptr(new LightStepRecorder(tracer, sink, dispatcher)); } void LightStepRecorder::flushSpans() { - sink_.tracerStats().spans_sent_.add(builder_.pendingSpans()); - lightstep::collector::ReportRequest request; - std::swap(request, builder_.pending()); - - Http::MessagePtr message = Grpc::Common::prepareHeaders( - sink_.collectorCluster(), LightStepSink::LIGHTSTEP_SERVICE, LightStepSink::LIGHTSTEP_METHOD); - - message->body(Grpc::Common::serializeBody(std::move(request))); - - sink_.clusterManager() - .httpAsyncClientForCluster(sink_.collectorCluster()) - .send(std::move(message), *this, std::chrono::milliseconds(5000)); + if (builder_.pendingSpans() != 0) { + sink_.tracerStats().spans_sent_.add(builder_.pendingSpans()); + lightstep::collector::ReportRequest request; + std::swap(request, builder_.pending()); + + Http::MessagePtr message = + Grpc::Common::prepareHeaders(sink_.collectorCluster(), LightStepSink::LIGHTSTEP_SERVICE, + LightStepSink::LIGHTSTEP_METHOD); + + message->body(Grpc::Common::serializeBody(std::move(request))); + + uint64_t timeout = + sink_.runtime().snapshot().getInteger("tracing.lightstep.request_timeout", 5000U); + sink_.clusterManager() + .httpAsyncClientForCluster(sink_.collectorCluster()) + .send(std::move(message), *this, std::chrono::milliseconds(timeout)); + } } LightStepSink::TlsLightStepTracer::TlsLightStepTracer(lightstep::Tracer tracer, LightStepSink& sink) @@ -183,10 +195,10 @@ LightStepSink::LightStepSink(const Json::Object& config, Upstream::ClusterManage fmt::format("{} collector cluster must support http2 for gRPC calls", collector_cluster_)); } - tls_.set(tls_slot_, [this](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectPtr { + tls_.set(tls_slot_, [this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectPtr { lightstep::Tracer tracer(lightstep::NewUserDefinedTransportLightStepTracer( - *options_, - std::bind(&LightStepRecorder::NewInstance, std::ref(*this), std::placeholders::_1))); + *options_, std::bind(&LightStepRecorder::NewInstance, std::ref(*this), std::ref(dispatcher), + std::placeholders::_1))); return ThreadLocal::ThreadLocalObjectPtr{new TlsLightStepTracer(std::move(tracer), *this)}; }); diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 7fc94c8229cc..59599d2fefac 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -146,7 +146,8 @@ class LightStepSink : public HttpSink { class LightStepRecorder : public lightstep::Recorder, Http::AsyncClient::Callbacks { public: - LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink); + LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepSink& sink, + Event::Dispatcher& dispatcher); // lightstep::Recorder void RecordSpan(lightstep::collector::Span&& span) override; @@ -158,6 +159,7 @@ class LightStepRecorder : public lightstep::Recorder, Http::AsyncClient::Callbac void flushSpans(); static std::unique_ptr NewInstance(LightStepSink& sink, + Event::Dispatcher& dispatcher, const lightstep::TracerImpl& tracer); private: From 66e8b60226f24ec03a3ed743cf1913c573816882 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 5 Oct 2016 12:24:59 -0700 Subject: [PATCH 5/6] Test fixes. --- test/common/tracing/http_tracer_impl_test.cc | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 84950cc08cbe..2c8aac8a5802 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -281,12 +281,12 @@ class LightStepSinkTest : public Test { .WillByDefault(ReturnRef(cm_.async_client_)); if (init_timer) { - timer_ = new NiceMock(&dispatcher_); + timer_ = new NiceMock(&tls_.dispatcher_); EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); } - sink_.reset(new LightStepSink(config, cm_, dispatcher_, stats_, "service_node", tls_, runtime_, - std::move(opts))); + sink_.reset(new LightStepSink(config, cm_, tls_.dispatcher_, stats_, "service_node", tls_, + runtime_, std::move(opts))); } void setupValidSink() { @@ -307,7 +307,6 @@ class LightStepSinkTest : public Test { NiceMock* timer_; Stats::IsolatedStoreImpl stats_; NiceMock cm_; - NiceMock dispatcher_; NiceMock cluster_; NiceMock random_; NiceMock runtime_; @@ -406,6 +405,8 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .Times(2) .WillRepeatedly(Return(2)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) + .WillOnce(Return(5000U)); sink_->flushTrace(empty_header_, empty_header_, request_info); sink_->flushTrace(empty_header_, empty_header_, request_info); @@ -461,6 +462,10 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { sink_->flushTrace(empty_header_, empty_header_, request_info); // Timer should be re-enabled. EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) + .WillOnce(Return(5000U)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.flush_interval_ms", 1000U)) + .WillOnce(Return(1000U)); timer_->callback_(); @@ -499,6 +504,8 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { EXPECT_CALL(request_info, protocol()).WillOnce(ReturnRef(protocol)); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .WillOnce(Return(1)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) + .WillOnce(Return(5000U)); sink_->flushTrace(empty_header_, empty_header_, request_info); From bf07f0c07297e4b2db701510475edbb983136b53 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 5 Oct 2016 13:17:26 -0700 Subject: [PATCH 6/6] Fix comments. --- source/common/tracing/http_tracer_impl.cc | 21 ++++++++++---------- source/common/tracing/http_tracer_impl.h | 11 +++++----- source/server/configuration_impl.cc | 2 +- test/common/tracing/http_tracer_impl_test.cc | 4 ++-- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 686895acce39..c71892520b7d 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -120,14 +120,10 @@ LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightS flush_timer_ = dispatcher.createTimer([this]() -> void { sink_.tracerStats().timer_flushed_.inc(); flushSpans(); - uint64_t flush_interval = - sink_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); - flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); + enableTimer(); }); - uint64_t flush_interval = - sink_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); - flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); + enableTimer(); } void LightStepRecorder::RecordSpan(lightstep::collector::Span&& span) { @@ -152,6 +148,12 @@ LightStepRecorder::NewInstance(LightStepSink& sink, Event::Dispatcher& dispatche return std::unique_ptr(new LightStepRecorder(tracer, sink, dispatcher)); } +void LightStepRecorder::enableTimer() { + uint64_t flush_interval = + sink_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); + flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); +} + void LightStepRecorder::flushSpans() { if (builder_.pendingSpans() != 0) { sink_.tracerStats().spans_sent_.add(builder_.pendingSpans()); @@ -176,12 +178,11 @@ LightStepSink::TlsLightStepTracer::TlsLightStepTracer(lightstep::Tracer tracer, : tracer_(tracer), sink_(sink) {} LightStepSink::LightStepSink(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Event::Dispatcher& dispatcher, Stats::Store& stats, - const std::string& service_node, ThreadLocal::Instance& tls, - Runtime::Loader& runtime, + Stats::Store& stats, const std::string& service_node, + ThreadLocal::Instance& tls, Runtime::Loader& runtime, std::unique_ptr options) : collector_cluster_(config.getString("collector_cluster")), cm_(cluster_manager), - dispatcher_(dispatcher), stats_store_(stats), + stats_store_(stats), tracer_stats_{LIGHTSTEP_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, service_node_(service_node), tls_(tls), runtime_(runtime), options_(std::move(options)), tls_slot_(tls.allocateSlot()) { diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 59599d2fefac..93b7cd697783 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -100,9 +100,8 @@ class HttpTracerImpl : public HttpTracer { class LightStepSink : public HttpSink { public: LightStepSink(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Event::Dispatcher& dispatcher, Stats::Store& stats, const std::string& service_node, - ThreadLocal::Instance& tls, Runtime::Loader& runtime, - std::unique_ptr options); + Stats::Store& stats, const std::string& service_node, ThreadLocal::Instance& tls, + Runtime::Loader& runtime, std::unique_ptr options); // Tracer::HttpSink void flushTrace(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, @@ -113,7 +112,6 @@ class LightStepSink : public HttpSink { Runtime::Loader& runtime() { return runtime_; } Stats::Store& statsStore() { return stats_store_; } LightstepTracerStats& tracerStats() { return tracer_stats_; } - Event::Dispatcher& dispatcher() { return dispatcher_; } static const std::string LIGHTSTEP_SERVICE; static const std::string LIGHTSTEP_METHOD; @@ -134,7 +132,6 @@ class LightStepSink : public HttpSink { const std::string collector_cluster_; Upstream::ClusterManager& cm_; - Event::Dispatcher& dispatcher_; Stats::Store& stats_store_; LightstepTracerStats tracer_stats_; const std::string service_node_; @@ -157,12 +154,14 @@ class LightStepRecorder : public lightstep::Recorder, Http::AsyncClient::Callbac void onSuccess(Http::MessagePtr&&) override; void onFailure(Http::AsyncClient::FailureReason) override; - void flushSpans(); static std::unique_ptr NewInstance(LightStepSink& sink, Event::Dispatcher& dispatcher, const lightstep::TracerImpl& tracer); private: + void enableTimer(); + void flushSpans(); + lightstep::ReportBuilder builder_; LightStepSink& sink_; Event::TimerPtr flush_timer_; diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 48273f7ef069..89fa9df5a351 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -88,7 +88,7 @@ void MainImpl::initializeTracers(const Json::Object& tracing_configuration_) { opts->guid_generator = [&rand]() { return rand.random(); }; http_tracer_->addSink(Tracing::HttpSinkPtr{new Tracing::LightStepSink( - sink.getObject("config"), *cluster_manager_, server_.dispatcher(), server_.stats(), + sink.getObject("config"), *cluster_manager_, server_.stats(), server_.options().serviceNodeName(), server_.threadLocal(), server_.runtime(), std::move(opts))}); } else { diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 2c8aac8a5802..4245df184701 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -285,8 +285,8 @@ class LightStepSinkTest : public Test { EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); } - sink_.reset(new LightStepSink(config, cm_, tls_.dispatcher_, stats_, "service_node", tls_, - runtime_, std::move(opts))); + sink_.reset( + new LightStepSink(config, cm_, stats_, "service_node", tls_, runtime_, std::move(opts))); } void setupValidSink() {