From c7322694c5be44eb8a97b2dbeebe7bd58cbe8493 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Thu, 25 Aug 2016 12:08:26 -0700 Subject: [PATCH 1/3] Add ability to differentiate traceable. --- source/common/http/access_log_impl.cc | 5 +- source/common/http/conn_manager_utility.cc | 17 +++- source/common/runtime/uuid_util.cc | 32 ++++++- source/common/runtime/uuid_util.h | 29 ++++-- source/common/tracing/http_tracer_impl.cc | 25 ++--- test/common/http/access_log_impl_test.cc | 26 +++-- test/common/http/conn_manager_utility_test.cc | 4 +- test/common/runtime/uuid_util_test.cc | 21 +++- test/common/tracing/http_tracer_impl_test.cc | 96 ++++++++----------- 9 files changed, 154 insertions(+), 101 deletions(-) diff --git a/source/common/http/access_log_impl.cc b/source/common/http/access_log_impl.cc index c94d88c00fff6..5c4e6ea47e4ac 100644 --- a/source/common/http/access_log_impl.cc +++ b/source/common/http/access_log_impl.cc @@ -76,7 +76,10 @@ FilterPtr FilterImpl::fromJson(Json::Object& json, Runtime::Loader& runtime) { TraceableRequestFilter::TraceableRequestFilter(Runtime::Loader& runtime) : runtime_(runtime) {} bool TraceableRequestFilter::evaluate(const RequestInfo& info, const HeaderMap& request_headers) { - return Tracing::HttpTracerUtility::isTracing(info, request_headers, runtime_).is_tracing; + Tracing::Decision decision = + Tracing::HttpTracerUtility::isTracing(info, request_headers, runtime_); + + return decision.is_tracing && decision.reason == Tracing::Reason::ServiceForced; } bool StatusCodeFilter::evaluate(const RequestInfo& info, const HeaderMap&) { diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 222f47f85640c..47127f26fb18c 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -102,7 +102,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea // If client sends x-client-trace-id we set x-request-id to be traceable for edge requests. if (edge_request && request_headers.has(Headers::get().ClientTraceId) && runtime.snapshot().featureEnabled("tracing.client_enabled", 100)) { - UuidUtils::setTraceableUuid(uuid); + UuidUtils::setTraceableUuid(uuid, TraceReason::Client); } request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); @@ -112,8 +112,21 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea // Make request traceable if x-envoy-force-trace header is set. if (request_headers.has(Headers::get().EnvoyForceTrace)) { std::string uuid = request_headers.get(Headers::get().RequestId); - UuidUtils::setTraceableUuid(uuid); + UuidUtils::setTraceableUuid(uuid, TraceReason::Forced); request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); + } else { // Check if we need to random sample request for tracing. + std::string uuid = request_headers.get(Http::Headers::get().RequestId); + uint16_t result; + + // Skip if x-request-id is corrupted. + if (!UuidUtils::uuidModBy(uuid, result, 10000)) { + return; + } + + if (runtime.snapshot().featureEnabled("tracing.random_sampling", 0, result, 10000)) { + UuidUtils::setTraceableUuid(uuid, TraceReason::Sampled); + request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); + } } } diff --git a/source/common/runtime/uuid_util.cc b/source/common/runtime/uuid_util.cc index fe5dc692089d8..9611b876d0e4e 100644 --- a/source/common/runtime/uuid_util.cc +++ b/source/common/runtime/uuid_util.cc @@ -17,17 +17,39 @@ bool UuidUtils::uuidModBy(const std::string& uuid, uint16_t& out, uint16_t mod) return true; } -bool UuidUtils::isTraceableUuid(const std::string& uuid) { - return uuid.length() == Runtime::RandomGeneratorImpl::UUID_LENGTH && - uuid[TRACE_BYTE_POSITION] == TRACE_BYTE_VALUE; +TraceDecision UuidUtils::isTraceableUuid(const std::string& uuid) { + if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) { + return {false, Optional()}; + } + + switch (uuid[TRACE_BYTE_POSITION]) { + case TRACE_FORCED: + return {true, Optional(TraceReason::Forced)}; + case TRACE_SAMPLED: + return {true, Optional(TraceReason::Sampled)}; + case TRACE_CLIENT: + return {true, Optional(TraceReason::Client)}; + default: + return {false, Optional()}; + } } -bool UuidUtils::setTraceableUuid(std::string& uuid) { +bool UuidUtils::setTraceableUuid(std::string& uuid, TraceReason trace_reason) { if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) { return false; } - uuid[TRACE_BYTE_POSITION] = TRACE_BYTE_VALUE; + switch (trace_reason) { + case TraceReason::Forced: + uuid[TRACE_BYTE_POSITION] = TRACE_FORCED; + break; + case TraceReason::Client: + uuid[TRACE_BYTE_POSITION] = TRACE_CLIENT; + break; + case TraceReason::Sampled: + uuid[TRACE_BYTE_POSITION] = TRACE_SAMPLED; + break; + } return true; } diff --git a/source/common/runtime/uuid_util.h b/source/common/runtime/uuid_util.h index 325912781dc0b..f16423b7b8427 100644 --- a/source/common/runtime/uuid_util.h +++ b/source/common/runtime/uuid_util.h @@ -1,5 +1,12 @@ #pragma once +enum class TraceReason { Sampled, Client, Forced }; + +struct TraceDecision { + bool is_traced; + Optional reason; +}; + /* * Utils for uuid4. */ @@ -16,19 +23,29 @@ class UuidUtils { /** * Modify uuid in a way it can be detected if uuid is traceable or not. * @param uuid is expected to be well formed uuid4. + * @param trace_reason is to specify why we modify uuid. * @return true on success, false on failure. */ - static bool setTraceableUuid(std::string& uuid); + static bool setTraceableUuid(std::string& uuid, TraceReason trace_reason); /** * @return bool to indicate if @param uuid is traceable uuid4. */ - static bool isTraceableUuid(const std::string& uuid); + static TraceDecision isTraceableUuid(const std::string& uuid); private: - // Byte on this position has predefined value of 4 for UUID4 + // Byte on this position has predefined value of 4 for UUID4. static const int TRACE_BYTE_POSITION = 14; - // Value of 9 is chosen randomly to distinguish between freshly generated uuid4 and the - // one modified so that we can detect if it's traceable. - static const char TRACE_BYTE_VALUE = '9'; + + // Value of '9' is chosen randomly to distinguish between freshly generated uuid4 and the + // one modified because we sample trace. + static const char TRACE_SAMPLED = '9'; + + // Value of 'a' is chosen randomly to distinguish between freshly generated uuid4 and the + // one modified because we force trace. + static const char TRACE_FORCED = 'a'; + + // Value of 'a' is chosen randomly to distinguish between freshly generated uuid4 and the + // one modified because of client trace. + static const char TRACE_CLIENT = 'b'; }; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index f545d119558e1..4b7310ed50398 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -28,28 +28,23 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques return {Reason::InvalidRequestId, false}; } - if (UuidUtils::isTraceableUuid(x_request_id)) { + TraceDecision x_rq_id_trace_decision = UuidUtils::isTraceableUuid(x_request_id); + + if (x_rq_id_trace_decision.is_traced) { + TraceReason trace_reason = x_rq_id_trace_decision.reason.value(); + if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) { return {Reason::GlobalSwitchOff, false}; } - if (request_headers.has(Http::Headers::get().ClientTraceId)) { + switch (trace_reason) { + case TraceReason::Client: return {Reason::ClientForced, true}; - } - - if (request_headers.has(Http::Headers::get().EnvoyForceTrace)) { + case TraceReason::Forced: return {Reason::ServiceForced, true}; + case TraceReason::Sampled: + return {Reason::Sampling, true}; } - - return {Reason::TraceableRequest, true}; - } - - if (runtime.snapshot().featureEnabled("tracing.random_sampling", 0, result, 10000)) { - if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) { - return {Reason::GlobalSwitchOff, false}; - } - - return {Reason::Sampling, true}; } return {Reason::NotTraceableRequestId, false}; diff --git a/test/common/http/access_log_impl_test.cc b/test/common/http/access_log_impl_test.cc index 33ca40cb920c7..7fa34d684428a 100644 --- a/test/common/http/access_log_impl_test.cc +++ b/test/common/http/access_log_impl_test.cc @@ -311,8 +311,12 @@ TEST_F(AccessLogImplTest, healthCheckFalse) { TEST_F(AccessLogImplTest, requestTracing) { Runtime::RandomGeneratorImpl random; std::string not_traceable_guid = random.uuid(); - std::string traceable_guid = random.uuid(); - UuidUtils::setTraceableUuid(traceable_guid); + + std::string force_tracing_guid = random.uuid(); + UuidUtils::setTraceableUuid(force_tracing_guid, TraceReason::Forced); + + std::string sample_tracing_guid = random.uuid(); + UuidUtils::setTraceableUuid(sample_tracing_guid, TraceReason::Sampled); std::string json = R"EOF( { @@ -327,22 +331,26 @@ TEST_F(AccessLogImplTest, requestTracing) { InstancePtr log = InstanceImpl::fromJson(loader, api_, dispatcher_, lock_, store, runtime); { - HeaderMapImpl traceable{{"x-request-id", traceable_guid}}; - EXPECT_CALL(*file_, write(_)); + HeaderMapImpl forced_header{{"x-request-id", force_tracing_guid}}; EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); - - log->log(&traceable, &response_headers_, request_info_); + EXPECT_CALL(*file_, write(_)); + log->log(&forced_header, &response_headers_, request_info_); } { HeaderMapImpl not_traceable{{"x-request-id", not_traceable_guid}}; EXPECT_CALL(*file_, write(_)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(false)); - log->log(¬_traceable, &response_headers_, request_info_); } + + { + HeaderMapImpl sampled_header{{"x-request-id", sample_tracing_guid}}; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); + EXPECT_CALL(*file_, write(_)).Times(0); + log->log(&sampled_header, &response_headers_, request_info_); + } } TEST(AccessLogImplTestCtor, OperatorIsNotSupported) { diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 88aaf36eb56e6..cc97720571621 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -99,7 +99,7 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) { {"x-envoy-force-trace", "true"}}; ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, random_, runtime_); - EXPECT_EQ("f4dca0a9-12c7-9307-8002-969403baf480", headers.get(Headers::get().RequestId)); + EXPECT_EQ("f4dca0a9-12c7-a307-8002-969403baf480", headers.get(Headers::get().RequestId)); } { @@ -165,7 +165,7 @@ TEST_F(ConnectionManagerUtilityTest, EdgeRequestRegenerateRequestIdAndWipeDownst runtime_); EXPECT_FALSE(headers.has(Headers::get().EnvoyDownstreamServiceCluster)); - EXPECT_EQ("f4dca0a9-12c7-9307-8002-969403baf480", headers.get(Headers::get().RequestId)); + EXPECT_EQ("f4dca0a9-12c7-b307-8002-969403baf480", headers.get(Headers::get().RequestId)); } } diff --git a/test/common/runtime/uuid_util_test.cc b/test/common/runtime/uuid_util_test.cc index 4ef3c4fe3d5e1..3770e25218893 100644 --- a/test/common/runtime/uuid_util_test.cc +++ b/test/common/runtime/uuid_util_test.cc @@ -57,11 +57,24 @@ TEST(UUIDUtilsTest, setAndCheckTraceable) { Runtime::RandomGeneratorImpl random; std::string uuid = random.uuid(); - EXPECT_FALSE(UuidUtils::isTraceableUuid(uuid)); + TraceDecision decision = UuidUtils::isTraceableUuid(uuid); + EXPECT_FALSE(decision.is_traced); - EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid)); - EXPECT_TRUE(UuidUtils::isTraceableUuid(uuid)); + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Sampled)); + decision = UuidUtils::isTraceableUuid(uuid); + EXPECT_TRUE(decision.is_traced); + EXPECT_EQ(decision.reason, Optional(TraceReason::Sampled)); + + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Client)); + decision = UuidUtils::isTraceableUuid(uuid); + EXPECT_TRUE(decision.is_traced); + EXPECT_EQ(decision.reason, Optional(TraceReason::Client)); + + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Forced)); + decision = UuidUtils::isTraceableUuid(uuid); + EXPECT_TRUE(decision.is_traced); + EXPECT_EQ(decision.reason, Optional(TraceReason::Forced)); std::string invalid_uuid = ""; - EXPECT_FALSE(UuidUtils::setTraceableUuid(invalid_uuid)); + EXPECT_FALSE(UuidUtils::setTraceableUuid(invalid_uuid, TraceReason::Forced)); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 7eb17b6f2c4ef..4b5c26140f04f 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -26,44 +26,51 @@ TEST(HttpTracerUtilityTest, IsTracing) { NiceMock stats; Runtime::RandomGeneratorImpl random; std::string not_traceable_guid = random.uuid(); - std::string traceable_guid = random.uuid(); - UuidUtils::setTraceableUuid(traceable_guid); - Http::HeaderMapImpl traceable_header{{"x-request-id", traceable_guid}}; + std::string forced_guid = random.uuid(); + UuidUtils::setTraceableUuid(forced_guid, TraceReason::Forced); + Http::HeaderMapImpl forced_header{{"x-request-id", forced_guid}}; + + std::string sampled_guid = random.uuid(); + UuidUtils::setTraceableUuid(sampled_guid, TraceReason::Sampled); + Http::HeaderMapImpl sampled_header{{"x-request-id", sampled_guid}}; + + std::string client_guid = random.uuid(); + UuidUtils::setTraceableUuid(client_guid, TraceReason::Client); + Http::HeaderMapImpl client_header{{"x-request-id", client_guid}}; + Http::HeaderMapImpl not_traceable_header{{"x-request-id", not_traceable_guid}}; Http::HeaderMapImpl empty_header{}; - // Global tracing enabled and x-request-id is traceable. + // Global tracing enabled and x-request-id is forced. { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, traceable_header, runtime); - EXPECT_EQ(Reason::TraceableRequest, result.reason); + Decision result = HttpTracerUtility::isTracing(request_info, forced_header, runtime); + EXPECT_EQ(Reason::ServiceForced, result.reason); EXPECT_TRUE(result.is_tracing); } - // Global tracing disabled and x-request-id is traceable. + // Global tracing disabled and x-request-id is force traced. { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(false)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, traceable_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, forced_header, runtime); EXPECT_EQ(Reason::GlobalSwitchOff, result.reason); EXPECT_FALSE(result.is_tracing); } - // x-request-id is not traceable, sampling is on, global is on. + // Global tracing enabled and x-request-id is sample traced. { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, not_traceable_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, sampled_header, runtime); EXPECT_EQ(Reason::Sampling, result.reason); EXPECT_TRUE(result.is_tracing); } @@ -71,9 +78,8 @@ TEST(HttpTracerUtilityTest, IsTracing) { // HC request. { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)).Times(0); - Http::HeaderMapImpl traceable_header_hc{{"x-request-id", traceable_guid}}; + Http::HeaderMapImpl traceable_header_hc{{"x-request-id", forced_guid}}; EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(true)); Decision result = HttpTracerUtility::isTracing(request_info, traceable_header_hc, runtime); @@ -81,15 +87,13 @@ TEST(HttpTracerUtilityTest, IsTracing) { EXPECT_FALSE(result.is_tracing); } - // x-request-id is not traceable, sampling is on, global is off. + // Sampled x-request-id, global is off. { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(false)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, not_traceable_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, sampled_header, runtime); EXPECT_EQ(Reason::GlobalSwitchOff, result.reason); EXPECT_FALSE(result.is_tracing); } @@ -98,27 +102,12 @@ TEST(HttpTracerUtilityTest, IsTracing) { { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); - Http::HeaderMapImpl traceable_header_client{{"x-request-id", traceable_guid}, - {"x-client-trace-id", random.uuid()}}; EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, traceable_header_client, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, client_header, runtime); EXPECT_EQ(Reason::ClientForced, result.reason); EXPECT_TRUE(result.is_tracing); } - - // x-request-id is traceable, service forced. - { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); - Http::HeaderMapImpl traceable_header_client{{"x-request-id", traceable_guid}, - {"x-envoy-force-trace", random.uuid()}}; - EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - - Decision result = HttpTracerUtility::isTracing(request_info, traceable_header_client, runtime); - EXPECT_EQ(Reason::ServiceForced, result.reason); - EXPECT_TRUE(result.is_tracing); - } } TEST(HttpTracerImplTest, AllSinksTraceableRequest) { @@ -126,10 +115,15 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { NiceMock stats; Runtime::RandomGeneratorImpl random; std::string not_traceable_guid = random.uuid(); - std::string traceable_guid = random.uuid(); - UuidUtils::setTraceableUuid(traceable_guid); - Http::HeaderMapImpl traceable_header{{"x-request-id", traceable_guid}}; + std::string forced_guid = random.uuid(); + UuidUtils::setTraceableUuid(forced_guid, TraceReason::Forced); + Http::HeaderMapImpl forced_header{{"x-request-id", forced_guid}}; + + std::string sampled_guid = random.uuid(); + UuidUtils::setTraceableUuid(sampled_guid, TraceReason::Sampled); + Http::HeaderMapImpl sampled_header{{"x-request-id", sampled_guid}}; + Http::HeaderMapImpl not_traceable_header{{"x-request-id", not_traceable_guid}}; Http::HeaderMapImpl empty_header{}; NiceMock request_info; @@ -147,7 +141,7 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { EXPECT_CALL(*sink2, flushTrace(_, _, _)); EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); - tracer.trace(&traceable_header, &empty_header, request_info); + tracer.trace(&forced_header, &empty_header, request_info); } // Global tracing disabled and x-request-id is traceable. @@ -157,19 +151,17 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(false)); - tracer.trace(&traceable_header, &empty_header, request_info); + tracer.trace(&forced_header, &empty_header, request_info); } - // x-request-id is not traceable, sampling is on, global is on. + // x-request-id is sample traced, global is on. { EXPECT_CALL(*sink1, flushTrace(_, _, _)); EXPECT_CALL(*sink2, flushTrace(_, _, _)); EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(true)); - tracer.trace(¬_traceable_header, &empty_header, request_info); + tracer.trace(&sampled_header, &empty_header, request_info); } // HC request. @@ -177,33 +169,28 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)).Times(0); - Http::HeaderMapImpl traceable_header_hc{{"x-request-id", traceable_guid}}; + Http::HeaderMapImpl traceable_header_hc{{"x-request-id", forced_guid}}; NiceMock request_info; EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(true)); tracer.trace(&traceable_header_hc, &empty_header, request_info); } - // x-request-id is not traceable, sampling is on, global is off. + // x-request-id is sample traced, global is off. { EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(false)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(true)); - tracer.trace(¬_traceable_header, &empty_header, request_info); + tracer.trace(&sampled_header, &empty_header, request_info); } - // x-request-id is not traceable, sampling is off, global not called. + // x-request-id is not traceable, global not called. { EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(false)); tracer.trace(¬_traceable_header, &empty_header, request_info); } @@ -218,11 +205,6 @@ TEST(HttpTracerImplTest, ZeroSinksRunsFine) { Http::HeaderMapImpl not_traceable{{"x-request-id", not_traceable_guid}}; - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) - .WillOnce(Return(true)); - NiceMock request_info; tracer.trace(¬_traceable, ¬_traceable, request_info); } From 86db499896f1741808dd85faf5470550c00067d7 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Fri, 26 Aug 2016 12:36:11 -0700 Subject: [PATCH 2/3] Refactor code. --- source/common/http/access_log_impl.cc | 3 +- source/common/http/conn_manager_utility.cc | 26 +----- source/common/runtime/uuid_util.cc | 25 +++--- source/common/runtime/uuid_util.h | 18 ++-- source/common/tracing/http_tracer_impl.cc | 74 ++++++++-------- source/common/tracing/http_tracer_impl.h | 10 ++- test/common/http/access_log_impl_test.cc | 8 +- test/common/http/conn_manager_utility_test.cc | 7 ++ test/common/runtime/uuid_util_test.cc | 26 +++--- test/common/tracing/http_tracer_impl_test.cc | 87 ++++--------------- 10 files changed, 103 insertions(+), 181 deletions(-) diff --git a/source/common/http/access_log_impl.cc b/source/common/http/access_log_impl.cc index 5c4e6ea47e4ac..1e2a6cc440ecb 100644 --- a/source/common/http/access_log_impl.cc +++ b/source/common/http/access_log_impl.cc @@ -76,8 +76,7 @@ FilterPtr FilterImpl::fromJson(Json::Object& json, Runtime::Loader& runtime) { TraceableRequestFilter::TraceableRequestFilter(Runtime::Loader& runtime) : runtime_(runtime) {} bool TraceableRequestFilter::evaluate(const RequestInfo& info, const HeaderMap& request_headers) { - Tracing::Decision decision = - Tracing::HttpTracerUtility::isTracing(info, request_headers, runtime_); + Tracing::Decision decision = Tracing::HttpTracerUtility::isTracing(info, request_headers); return decision.is_tracing && decision.reason == Tracing::Reason::ServiceForced; } diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 47127f26fb18c..a51f75981c923 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -99,35 +99,11 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea } if (!uuid.empty()) { - // If client sends x-client-trace-id we set x-request-id to be traceable for edge requests. - if (edge_request && request_headers.has(Headers::get().ClientTraceId) && - runtime.snapshot().featureEnabled("tracing.client_enabled", 100)) { - UuidUtils::setTraceableUuid(uuid, TraceReason::Client); - } - request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); } } - // Make request traceable if x-envoy-force-trace header is set. - if (request_headers.has(Headers::get().EnvoyForceTrace)) { - std::string uuid = request_headers.get(Headers::get().RequestId); - UuidUtils::setTraceableUuid(uuid, TraceReason::Forced); - request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); - } else { // Check if we need to random sample request for tracing. - std::string uuid = request_headers.get(Http::Headers::get().RequestId); - uint16_t result; - - // Skip if x-request-id is corrupted. - if (!UuidUtils::uuidModBy(uuid, result, 10000)) { - return; - } - - if (runtime.snapshot().featureEnabled("tracing.random_sampling", 0, result, 10000)) { - UuidUtils::setTraceableUuid(uuid, TraceReason::Sampled); - request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid)); - } - } + Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime); } void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers, diff --git a/source/common/runtime/uuid_util.cc b/source/common/runtime/uuid_util.cc index 9611b876d0e4e..1f1a22b37030c 100644 --- a/source/common/runtime/uuid_util.cc +++ b/source/common/runtime/uuid_util.cc @@ -17,38 +17,41 @@ bool UuidUtils::uuidModBy(const std::string& uuid, uint16_t& out, uint16_t mod) return true; } -TraceDecision UuidUtils::isTraceableUuid(const std::string& uuid) { +UuidTraceStatus UuidUtils::isTraceableUuid(const std::string& uuid) { if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) { - return {false, Optional()}; + return UuidTraceStatus::NoTrace; } switch (uuid[TRACE_BYTE_POSITION]) { case TRACE_FORCED: - return {true, Optional(TraceReason::Forced)}; + return UuidTraceStatus::Forced; case TRACE_SAMPLED: - return {true, Optional(TraceReason::Sampled)}; + return UuidTraceStatus::Sampled; case TRACE_CLIENT: - return {true, Optional(TraceReason::Client)}; + return UuidTraceStatus::Client; default: - return {false, Optional()}; + return UuidTraceStatus::NoTrace; } } -bool UuidUtils::setTraceableUuid(std::string& uuid, TraceReason trace_reason) { +bool UuidUtils::setTraceableUuid(std::string& uuid, UuidTraceStatus trace_status) { if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) { return false; } - switch (trace_reason) { - case TraceReason::Forced: + switch (trace_status) { + case UuidTraceStatus::Forced: uuid[TRACE_BYTE_POSITION] = TRACE_FORCED; break; - case TraceReason::Client: + case UuidTraceStatus::Client: uuid[TRACE_BYTE_POSITION] = TRACE_CLIENT; break; - case TraceReason::Sampled: + case UuidTraceStatus::Sampled: uuid[TRACE_BYTE_POSITION] = TRACE_SAMPLED; break; + case UuidTraceStatus::NoTrace: + uuid[TRACE_BYTE_POSITION] = NO_TRACE; + break; } return true; diff --git a/source/common/runtime/uuid_util.h b/source/common/runtime/uuid_util.h index f16423b7b8427..c4b9210435970 100644 --- a/source/common/runtime/uuid_util.h +++ b/source/common/runtime/uuid_util.h @@ -1,11 +1,6 @@ #pragma once -enum class TraceReason { Sampled, Client, Forced }; - -struct TraceDecision { - bool is_traced; - Optional reason; -}; +enum class UuidTraceStatus { NoTrace, Sampled, Client, Forced }; /* * Utils for uuid4. @@ -23,15 +18,15 @@ class UuidUtils { /** * Modify uuid in a way it can be detected if uuid is traceable or not. * @param uuid is expected to be well formed uuid4. - * @param trace_reason is to specify why we modify uuid. + * @param trace_status is to specify why we modify uuid. * @return true on success, false on failure. */ - static bool setTraceableUuid(std::string& uuid, TraceReason trace_reason); + static bool setTraceableUuid(std::string& uuid, UuidTraceStatus trace_status); /** - * @return bool to indicate if @param uuid is traceable uuid4. + * @return status of the uuid, to differentiate reason for tracing, etc. */ - static TraceDecision isTraceableUuid(const std::string& uuid); + static UuidTraceStatus isTraceableUuid(const std::string& uuid); private: // Byte on this position has predefined value of 4 for UUID4. @@ -48,4 +43,7 @@ class UuidUtils { // Value of 'a' is chosen randomly to distinguish between freshly generated uuid4 and the // one modified because of client trace. static const char TRACE_CLIENT = 'b'; + + // Initial value for freshly generated uuid4. + static const char NO_TRACE = '4'; }; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 4b7310ed50398..3bee44094fb43 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -12,42 +12,53 @@ namespace Tracing { -Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& request_info, - const Http::HeaderMap& request_headers, - Runtime::Loader& runtime) { - // Exclude HC requests immediately. - if (request_info.healthCheck()) { - return {Reason::HealthCheck, false}; - } +void HttpTracerUtility::mutateHeaders(Http::HeaderMap& request_headers, Runtime::Loader& runtime) { + std::string x_request_id = request_headers.get(Http::Headers::get().RequestId); - const std::string& x_request_id = request_headers.get(Http::Headers::get().RequestId); uint16_t result; - - // If x-request-id is corrupted then return not tracing immediately. + // Skip if x-request-id is corrupted. if (!UuidUtils::uuidModBy(x_request_id, result, 10000)) { - return {Reason::InvalidRequestId, false}; + return; } - TraceDecision x_rq_id_trace_decision = UuidUtils::isTraceableUuid(x_request_id); + if (request_headers.has(Http::Headers::get().ClientTraceId) && + runtime.snapshot().featureEnabled("tracing.client_enabled", 100)) { + UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::Client); + } else if (request_headers.has(Http::Headers::get().EnvoyForceTrace)) { + UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::Forced); + } else if (runtime.snapshot().featureEnabled("tracing.random_sampling", 0, result, 10000)) { + UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::Sampled); + } - if (x_rq_id_trace_decision.is_traced) { - TraceReason trace_reason = x_rq_id_trace_decision.reason.value(); + if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) { + UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::NoTrace); + } - if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) { - return {Reason::GlobalSwitchOff, false}; - } + request_headers.replaceViaCopy(Http::Headers::get().RequestId, x_request_id); +} - switch (trace_reason) { - case TraceReason::Client: - return {Reason::ClientForced, true}; - case TraceReason::Forced: - return {Reason::ServiceForced, true}; - case TraceReason::Sampled: - return {Reason::Sampling, true}; - } +Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& request_info, + const Http::HeaderMap& request_headers) { + // Exclude HC requests immediately. + if (request_info.healthCheck()) { + return {Reason::HealthCheck, false}; } - return {Reason::NotTraceableRequestId, false}; + UuidTraceStatus trace_status = + UuidUtils::isTraceableUuid(request_headers.get(Http::Headers::get().RequestId)); + + switch (trace_status) { + case UuidTraceStatus::Client: + return {Reason::ClientForced, true}; + case UuidTraceStatus::Forced: + return {Reason::ServiceForced, true}; + case UuidTraceStatus::Sampled: + return {Reason::Sampling, true}; + case UuidTraceStatus::NoTrace: + return {Reason::NotTraceableRequestId, false}; + } + + throw std::invalid_argument("Unknown trace_status"); } HttpTracerImpl::HttpTracerImpl(Runtime::Loader& runtime, Stats::Store& stats) @@ -69,7 +80,7 @@ void HttpTracerImpl::trace(const Http::HeaderMap* request_headers, stats_.flush_.inc(); - Decision decision = HttpTracerUtility::isTracing(request_info, *request_headers, runtime_); + Decision decision = HttpTracerUtility::isTracing(request_info, *request_headers); populateStats(decision); if (decision.is_tracing) { @@ -86,15 +97,9 @@ void HttpTracerImpl::populateStats(const Decision& decision) { case Reason::ClientForced: stats_.client_enabled_.inc(); break; - case Reason::GlobalSwitchOff: - stats_.global_switch_off_.inc(); - break; case Reason::HealthCheck: stats_.health_check_.inc(); break; - case Reason::InvalidRequestId: - stats_.invalid_request_id_.inc(); - break; case Reason::NotTraceableRequestId: stats_.not_traceable_.inc(); break; @@ -104,9 +109,6 @@ void HttpTracerImpl::populateStats(const Decision& decision) { case Reason::ServiceForced: stats_.service_forced_.inc(); break; - case Reason::TraceableRequest: - stats_.traceable_.inc(); - break; } } diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index ca281e09e8251..b7df2db3dd858 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -42,14 +42,11 @@ class HttpNullTracer : public HttpTracer { }; enum class Reason { - InvalidRequestId, NotTraceableRequestId, HealthCheck, - GlobalSwitchOff, Sampling, ServiceForced, ClientForced, - TraceableRequest }; struct Decision { @@ -66,7 +63,12 @@ class HttpTracerUtility { * @return decision if request is traceable or not and Reason why. **/ static Decision isTracing(const Http::AccessLog::RequestInfo& request_info, - const Http::HeaderMap& request_headers, Runtime::Loader& runtime); + const Http::HeaderMap& request_headers); + + /** + * Mutate request headers if request needs to be traced. + */ + static void mutateHeaders(Http::HeaderMap& request_headers, Runtime::Loader& runtime); }; class HttpTracerImpl : public HttpTracer { diff --git a/test/common/http/access_log_impl_test.cc b/test/common/http/access_log_impl_test.cc index 7fa34d684428a..444828bfaf394 100644 --- a/test/common/http/access_log_impl_test.cc +++ b/test/common/http/access_log_impl_test.cc @@ -313,10 +313,10 @@ TEST_F(AccessLogImplTest, requestTracing) { std::string not_traceable_guid = random.uuid(); std::string force_tracing_guid = random.uuid(); - UuidUtils::setTraceableUuid(force_tracing_guid, TraceReason::Forced); + UuidUtils::setTraceableUuid(force_tracing_guid, UuidTraceStatus::Forced); std::string sample_tracing_guid = random.uuid(); - UuidUtils::setTraceableUuid(sample_tracing_guid, TraceReason::Sampled); + UuidUtils::setTraceableUuid(sample_tracing_guid, UuidTraceStatus::Sampled); std::string json = R"EOF( { @@ -332,8 +332,6 @@ TEST_F(AccessLogImplTest, requestTracing) { { HeaderMapImpl forced_header{{"x-request-id", force_tracing_guid}}; - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); EXPECT_CALL(*file_, write(_)); log->log(&forced_header, &response_headers_, request_info_); } @@ -346,8 +344,6 @@ TEST_F(AccessLogImplTest, requestTracing) { { HeaderMapImpl sampled_header{{"x-request-id", sample_tracing_guid}}; - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); EXPECT_CALL(*file_, write(_)).Times(0); log->log(&sampled_header, &response_headers_, request_info_); } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index cc97720571621..d4cc221887f85 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -97,8 +97,11 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) { HeaderMapImpl headers{{"x-forwarded-for", internal_remote_address}, {"x-request-id", uuid}, {"x-envoy-force-trace", "true"}}; + EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, random_, runtime_); + EXPECT_EQ("f4dca0a9-12c7-a307-8002-969403baf480", headers.get(Headers::get().RequestId)); } @@ -107,6 +110,8 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) { HeaderMapImpl headers{{"x-forwarded-for", external_remote_address}, {"x-request-id", uuid}, {"x-envoy-force-trace", "true"}}; + EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, random_, runtime_); EXPECT_EQ(uuid, headers.get(Headers::get().RequestId)); @@ -120,6 +125,8 @@ TEST_F(ConnectionManagerUtilityTest, EdgeRequestRegenerateRequestIdAndWipeDownst ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true)); ON_CALL(connection_, remoteAddress()).WillByDefault(ReturnRef(external_remote_address)); + ON_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillByDefault(Return(true)); { HeaderMapImpl headers{{"x-envoy-downstream-service-cluster", "foo"}, diff --git a/test/common/runtime/uuid_util_test.cc b/test/common/runtime/uuid_util_test.cc index 3770e25218893..b8cdf9a080c4f 100644 --- a/test/common/runtime/uuid_util_test.cc +++ b/test/common/runtime/uuid_util_test.cc @@ -57,24 +57,20 @@ TEST(UUIDUtilsTest, setAndCheckTraceable) { Runtime::RandomGeneratorImpl random; std::string uuid = random.uuid(); - TraceDecision decision = UuidUtils::isTraceableUuid(uuid); - EXPECT_FALSE(decision.is_traced); + EXPECT_EQ(UuidTraceStatus::NoTrace, UuidUtils::isTraceableUuid(uuid)); - EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Sampled)); - decision = UuidUtils::isTraceableUuid(uuid); - EXPECT_TRUE(decision.is_traced); - EXPECT_EQ(decision.reason, Optional(TraceReason::Sampled)); + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::Sampled)); + EXPECT_EQ(UuidTraceStatus::Sampled, UuidUtils::isTraceableUuid(uuid)); - EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Client)); - decision = UuidUtils::isTraceableUuid(uuid); - EXPECT_TRUE(decision.is_traced); - EXPECT_EQ(decision.reason, Optional(TraceReason::Client)); + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::Client)); + EXPECT_EQ(UuidTraceStatus::Client, UuidUtils::isTraceableUuid(uuid)); - EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, TraceReason::Forced)); - decision = UuidUtils::isTraceableUuid(uuid); - EXPECT_TRUE(decision.is_traced); - EXPECT_EQ(decision.reason, Optional(TraceReason::Forced)); + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::Forced)); + EXPECT_EQ(UuidTraceStatus::Forced, UuidUtils::isTraceableUuid(uuid)); + + EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::NoTrace)); + EXPECT_EQ(UuidTraceStatus::NoTrace, UuidUtils::isTraceableUuid(uuid)); std::string invalid_uuid = ""; - EXPECT_FALSE(UuidUtils::setTraceableUuid(invalid_uuid, TraceReason::Forced)); + EXPECT_FALSE(UuidUtils::setTraceableUuid(invalid_uuid, UuidTraceStatus::Forced)); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 4b5c26140f04f..68c84ae7b15e5 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -22,89 +22,58 @@ namespace Tracing { TEST(HttpTracerUtilityTest, IsTracing) { NiceMock request_info; - NiceMock runtime; NiceMock stats; Runtime::RandomGeneratorImpl random; std::string not_traceable_guid = random.uuid(); std::string forced_guid = random.uuid(); - UuidUtils::setTraceableUuid(forced_guid, TraceReason::Forced); + UuidUtils::setTraceableUuid(forced_guid, UuidTraceStatus::Forced); Http::HeaderMapImpl forced_header{{"x-request-id", forced_guid}}; std::string sampled_guid = random.uuid(); - UuidUtils::setTraceableUuid(sampled_guid, TraceReason::Sampled); + UuidUtils::setTraceableUuid(sampled_guid, UuidTraceStatus::Sampled); Http::HeaderMapImpl sampled_header{{"x-request-id", sampled_guid}}; std::string client_guid = random.uuid(); - UuidUtils::setTraceableUuid(client_guid, TraceReason::Client); + UuidUtils::setTraceableUuid(client_guid, UuidTraceStatus::Client); Http::HeaderMapImpl client_header{{"x-request-id", client_guid}}; Http::HeaderMapImpl not_traceable_header{{"x-request-id", not_traceable_guid}}; Http::HeaderMapImpl empty_header{}; - // Global tracing enabled and x-request-id is forced. + // Force traced. { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, forced_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, forced_header); EXPECT_EQ(Reason::ServiceForced, result.reason); EXPECT_TRUE(result.is_tracing); } - // Global tracing disabled and x-request-id is force traced. - { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(false)); - EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - - Decision result = HttpTracerUtility::isTracing(request_info, forced_header, runtime); - EXPECT_EQ(Reason::GlobalSwitchOff, result.reason); - EXPECT_FALSE(result.is_tracing); - } - - // Global tracing enabled and x-request-id is sample traced. + // Sample traced. { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, sampled_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, sampled_header); EXPECT_EQ(Reason::Sampling, result.reason); EXPECT_TRUE(result.is_tracing); } // HC request. { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); - Http::HeaderMapImpl traceable_header_hc{{"x-request-id", forced_guid}}; EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(true)); - Decision result = HttpTracerUtility::isTracing(request_info, traceable_header_hc, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, traceable_header_hc); EXPECT_EQ(Reason::HealthCheck, result.reason); EXPECT_FALSE(result.is_tracing); } - // Sampled x-request-id, global is off. - { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(false)); - EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - - Decision result = HttpTracerUtility::isTracing(request_info, sampled_header, runtime); - EXPECT_EQ(Reason::GlobalSwitchOff, result.reason); - EXPECT_FALSE(result.is_tracing); - } - - // x-request-id is traceable, client called. + // Client traced. { - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); EXPECT_CALL(request_info, healthCheck()).WillOnce(Return(false)); - Decision result = HttpTracerUtility::isTracing(request_info, client_header, runtime); + Decision result = HttpTracerUtility::isTracing(request_info, client_header); EXPECT_EQ(Reason::ClientForced, result.reason); EXPECT_TRUE(result.is_tracing); } @@ -117,11 +86,11 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { std::string not_traceable_guid = random.uuid(); std::string forced_guid = random.uuid(); - UuidUtils::setTraceableUuid(forced_guid, TraceReason::Forced); + UuidUtils::setTraceableUuid(forced_guid, UuidTraceStatus::Forced); Http::HeaderMapImpl forced_header{{"x-request-id", forced_guid}}; std::string sampled_guid = random.uuid(); - UuidUtils::setTraceableUuid(sampled_guid, TraceReason::Sampled); + UuidUtils::setTraceableUuid(sampled_guid, UuidTraceStatus::Sampled); Http::HeaderMapImpl sampled_header{{"x-request-id", sampled_guid}}; Http::HeaderMapImpl not_traceable_header{{"x-request-id", not_traceable_guid}}; @@ -135,31 +104,17 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { tracer.addSink(HttpSinkPtr{sink1}); tracer.addSink(HttpSinkPtr{sink2}); - // Global tracing enabled and x-request-id is traceable. + // Force traced request. { EXPECT_CALL(*sink1, flushTrace(_, _, _)); EXPECT_CALL(*sink2, flushTrace(_, _, _)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); tracer.trace(&forced_header, &empty_header, request_info); } - // Global tracing disabled and x-request-id is traceable. - { - EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(false)); - - tracer.trace(&forced_header, &empty_header, request_info); - } - - // x-request-id is sample traced, global is on. + // x-request-id is sample traced. { EXPECT_CALL(*sink1, flushTrace(_, _, _)); EXPECT_CALL(*sink2, flushTrace(_, _, _)); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(true)); tracer.trace(&sampled_header, &empty_header, request_info); } @@ -168,7 +123,6 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { { EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); Http::HeaderMapImpl traceable_header_hc{{"x-request-id", forced_guid}}; NiceMock request_info; @@ -176,21 +130,10 @@ TEST(HttpTracerImplTest, AllSinksTraceableRequest) { tracer.trace(&traceable_header_hc, &empty_header, request_info); } - // x-request-id is sample traced, global is off. - { - EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) - .WillOnce(Return(false)); - - tracer.trace(&sampled_header, &empty_header, request_info); - } - - // x-request-id is not traceable, global not called. + // x-request-id is not traceable. { EXPECT_CALL(*sink1, flushTrace(_, _, _)).Times(0); EXPECT_CALL(*sink2, flushTrace(_, _, _)).Times(0); - EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)).Times(0); tracer.trace(¬_traceable_header, &empty_header, request_info); } From e34f43b9ec3a419c9d62afb05445af193d0b8d01 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Fri, 26 Aug 2016 14:10:34 -0700 Subject: [PATCH 3/3] Add more tests. --- test/common/tracing/http_tracer_impl_test.cc | 105 +++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 68c84ae7b15e5..b21ca8340c164 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -20,6 +20,111 @@ using testing::Test; namespace Tracing { +TEST(HttpTracerUtilityTest, mutateHeaders) { + // Sampling, global on. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) + .WillOnce(Return(true)); + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); + + Http::HeaderMapImpl request_headers{{"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::Sampled, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Sampling, global off. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.random_sampling", 0, _, 10000)) + .WillOnce(Return(true)); + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(false)); + + Http::HeaderMapImpl request_headers{{"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::NoTrace, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Client, client enabled, global on. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.client_enabled", 100)) + .WillOnce(Return(true)); + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); + + Http::HeaderMapImpl request_headers{ + {"x-client-trace-id", "f4dca0a9-12c7-4307-8002-969403baf480"}, + {"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::Client, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Client, client disabled, global on. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.client_enabled", 100)) + .WillOnce(Return(false)); + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); + + Http::HeaderMapImpl request_headers{ + {"x-client-trace-id", "f4dca0a9-12c7-4307-8002-969403baf480"}, + {"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::NoTrace, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Forced, global on. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(true)); + + Http::HeaderMapImpl request_headers{{"x-envoy-force-trace", "true"}, + {"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::Forced, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Forced, global off. + { + NiceMock runtime; + EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) + .WillOnce(Return(false)); + + Http::HeaderMapImpl request_headers{{"x-envoy-force-trace", "true"}, + {"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::NoTrace, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } + + // Forced, global on, broken uuid. + { + NiceMock runtime; + + Http::HeaderMapImpl request_headers{{"x-envoy-force-trace", "true"}, {"x-request-id", "bb"}}; + HttpTracerUtility::mutateHeaders(request_headers, runtime); + + EXPECT_EQ(UuidTraceStatus::NoTrace, + UuidUtils::isTraceableUuid(request_headers.get("x-request-id"))); + } +} + TEST(HttpTracerUtilityTest, IsTracing) { NiceMock request_info; NiceMock stats;