diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 085dd1f426391..c0c414f9e3788 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -264,6 +264,27 @@ void ConnectionManagerImpl::onDrainTimeout() { checkForDeferredClose(); } +void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_reason, + ConnectionManagerTracingStats& tracing_stats) { + switch (tracing_reason) { + case Tracing::Reason::ClientForced: + tracing_stats.client_enabled_.inc(); + break; + case Tracing::Reason::NotTraceableRequestId: + tracing_stats.not_traceable_.inc(); + break; + case Tracing::Reason::Sampling: + tracing_stats.random_sampling_.inc(); + break; + case Tracing::Reason::ServiceForced: + tracing_stats.service_forced_.inc(); + break; + default: + throw std::invalid_argument( + fmt::format("invalid tracing reason, value: {}", static_cast(tracing_reason))); + } +} + ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager) : connection_manager_(connection_manager), stream_id_(ConnectionManagerUtility::generateStreamId( @@ -288,8 +309,12 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { log_handler->log(request_headers_.get(), response_headers_.get(), request_info_); } - if (active_span_ && !request_info_.healthCheck()) { - Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_info_); + if (active_span_) { + if (request_info_.healthCheck()) { + connection_manager_.config_.tracingStats().health_check_.inc(); + } else { + Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_info_); + } } } @@ -334,27 +359,6 @@ void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) { } } -void ConnectionManagerImpl::ActiveStream::chargeTracingStats( - const Tracing::Decision& tracing_decision) { - switch (tracing_decision.reason) { - case Tracing::Reason::ClientForced: - connection_manager_.config_.tracingStats().client_enabled_.inc(); - break; - case Tracing::Reason::HealthCheck: - connection_manager_.config_.tracingStats().health_check_.inc(); - break; - case Tracing::Reason::NotTraceableRequestId: - connection_manager_.config_.tracingStats().not_traceable_.inc(); - break; - case Tracing::Reason::Sampling: - connection_manager_.config_.tracingStats().random_sampling_.inc(); - break; - case Tracing::Reason::ServiceForced: - connection_manager_.config_.tracingStats().service_forced_.inc(); - break; - } -} - uint64_t ConnectionManagerImpl::ActiveStream::connectionId() { return connection_manager_.read_callbacks_->connection().id(); } @@ -437,7 +441,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (connection_manager_.config_.tracingConfig().valid()) { Tracing::Decision tracing_decision = Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); - chargeTracingStats(tracing_decision); + ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, + connection_manager_.config_.tracingStats()); if (tracing_decision.is_tracing) { active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index dbed9dc089b3d..be3f161e5f8f3 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -221,6 +221,8 @@ class ConnectionManagerImpl : Logger::Loggable, static ConnectionManagerStats generateStats(const std::string& prefix, Stats::Store& stats); static ConnectionManagerTracingStats generateTracingStats(const std::string& prefix, Stats::Store& stats); + static void chargeTracingStats(const Tracing::Reason& tracing_reason, + ConnectionManagerTracingStats& tracing_stats); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data) override; @@ -355,7 +357,6 @@ class ConnectionManagerImpl : Logger::Loggable, ~ActiveStream(); void chargeStats(HeaderMap& headers); - void chargeTracingStats(const Tracing::Decision& tracing_decision); std::list::iterator commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_stream); uint64_t connectionId(); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ee40a4d04c9a3..4ba9e530df0b0 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -208,7 +208,13 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { setup(false, ""); NiceMock* span = new NiceMock(); - EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); + EXPECT_CALL(tracer_, startSpan_(_, _, _)) + .WillOnce(Invoke([&](const Tracing::Config& config, const Http::HeaderMap&, + const Http::AccessLog::RequestInfo&) -> Tracing::Span* { + EXPECT_EQ("operation", config.operationName()); + + return span; + })); EXPECT_CALL(*span, finishSpan()); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -240,6 +246,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); + + EXPECT_EQ(1UL, tracing_stats_.service_forced_.value()); + EXPECT_EQ(0UL, tracing_stats_.random_sampling_.value()); } TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { @@ -355,7 +364,7 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) { Http::HeaderMapPtr headers{ new TestHeaderMapImpl{{":authority", "host"}, {":path", "/healthcheck"}, - {"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}}; + {"x-request-id", "125a4afb-6f55-94ba-ad80-413f09f48a28"}}}; decoder->decodeHeaders(std::move(headers), true); Http::HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; @@ -366,6 +375,14 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); + + // Force dtor of active stream to be called so that we can capture tracing HC stat. + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + + // HC request, but was originally sampled, so check for two stats here. + EXPECT_EQ(1UL, tracing_stats_.random_sampling_.value()); + EXPECT_EQ(1UL, tracing_stats_.health_check_.value()); + EXPECT_EQ(0UL, tracing_stats_.service_forced_.value()); } TEST_F(HttpConnectionManagerImplTest, NoPath) { @@ -865,4 +882,19 @@ TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { encoder_filter1->callbacks_->continueEncoding(); } +TEST(HttpConnectionManagerTracingStatsTest, verifyTracingStats) { + Stats::IsolatedStoreImpl stats; + ConnectionManagerTracingStats tracing_stats{CONN_MAN_TRACING_STATS(POOL_COUNTER(stats))}; + + EXPECT_THROW( + ConnectionManagerImpl::chargeTracingStats(Tracing::Reason::HealthCheck, tracing_stats), + std::invalid_argument); + + ConnectionManagerImpl::chargeTracingStats(Tracing::Reason::ClientForced, tracing_stats); + EXPECT_EQ(1UL, tracing_stats.client_enabled_.value()); + + ConnectionManagerImpl::chargeTracingStats(Tracing::Reason::NotTraceableRequestId, tracing_stats); + EXPECT_EQ(1UL, tracing_stats.not_traceable_.value()); +} + } // Http