Differentiate in x-request-id value what made it to be traceable.

source/common/http/access_log_impl.cc

@@ -76,7 +76,9 @@ 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);
+
+  return decision.is_tracing && decision.reason == Tracing::Reason::ServiceForced;
 }
 
 bool StatusCodeFilter::evaluate(const RequestInfo& info, const HeaderMap&) {

source/common/http/conn_manager_utility.cc

@@ -99,22 +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);
-      }
-
       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);
-    request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid));
-  }
+  Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime);
 }
 
 void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers,

source/common/runtime/uuid_util.cc

@@ -17,17 +17,42 @@ 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;
+UuidTraceStatus UuidUtils::isTraceableUuid(const std::string& uuid) {
+  if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) {
+    return UuidTraceStatus::NoTrace;
+  }
+
+  switch (uuid[TRACE_BYTE_POSITION]) {
+  case TRACE_FORCED:
+    return UuidTraceStatus::Forced;
+  case TRACE_SAMPLED:
+    return UuidTraceStatus::Sampled;
+  case TRACE_CLIENT:
+    return UuidTraceStatus::Client;
+  default:
+    return UuidTraceStatus::NoTrace;
+  }
 }
 
-bool UuidUtils::setTraceableUuid(std::string& uuid) {
+bool UuidUtils::setTraceableUuid(std::string& uuid, UuidTraceStatus trace_status) {
   if (uuid.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) {
     return false;
   }
 
-  uuid[TRACE_BYTE_POSITION] = TRACE_BYTE_VALUE;
+  switch (trace_status) {
+  case UuidTraceStatus::Forced:
+    uuid[TRACE_BYTE_POSITION] = TRACE_FORCED;
+    break;
+  case UuidTraceStatus::Client:
+    uuid[TRACE_BYTE_POSITION] = TRACE_CLIENT;
+    break;
+  case UuidTraceStatus::Sampled:
+    uuid[TRACE_BYTE_POSITION] = TRACE_SAMPLED;
+    break;
+  case UuidTraceStatus::NoTrace:
+    uuid[TRACE_BYTE_POSITION] = NO_TRACE;
+    break;
+  }
 
   return true;
 }

source/common/runtime/uuid_util.h

@@ -1,5 +1,7 @@
 #pragma once
 
+enum class UuidTraceStatus { NoTrace, Sampled, Client, Forced };
+
 /*
  * Utils for uuid4.
  */
@@ -16,19 +18,32 @@ 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_status 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, 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 bool isTraceableUuid(const std::string& uuid);
+  static UuidTraceStatus 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';
+
+  // Initial value for freshly generated uuid4.
+  static const char NO_TRACE = '4';
 };

source/common/tracing/http_tracer_impl.cc

@@ -12,47 +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;
   }
 
-  if (UuidUtils::isTraceableUuid(x_request_id)) {
-    if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) {
-      return {Reason::GlobalSwitchOff, false};
-    }
+  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 (request_headers.has(Http::Headers::get().ClientTraceId)) {
-      return {Reason::ClientForced, true};
-    }
+  if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) {
+    UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::NoTrace);
+  }
 
-    if (request_headers.has(Http::Headers::get().EnvoyForceTrace)) {
-      return {Reason::ServiceForced, true};
-    }
+  request_headers.replaceViaCopy(Http::Headers::get().RequestId, x_request_id);
+}
 
-    return {Reason::TraceableRequest, 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};
   }
 
-  if (runtime.snapshot().featureEnabled("tracing.random_sampling", 0, result, 10000)) {
-    if (!runtime.snapshot().featureEnabled("tracing.global_enabled", 100, result)) {
-      return {Reason::GlobalSwitchOff, 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};
   }
 
-  return {Reason::NotTraceableRequestId, false};
+  throw std::invalid_argument("Unknown trace_status");
 }
 
 HttpTracerImpl::HttpTracerImpl(Runtime::Loader& runtime, Stats::Store& stats)
@@ -74,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) {
@@ -91,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;
@@ -109,9 +109,6 @@ void HttpTracerImpl::populateStats(const Decision& decision) {
   case Reason::ServiceForced:
     stats_.service_forced_.inc();
     break;
-  case Reason::TraceableRequest:
-    stats_.traceable_.inc();
-    break;
   }
 }
 

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 {

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, UuidTraceStatus::Forced);
+
+  std::string sample_tracing_guid = random.uuid();
+  UuidUtils::setTraceableUuid(sample_tracing_guid, UuidTraceStatus::Sampled);
 
   std::string json = R"EOF(
   {
@@ -327,22 +331,22 @@ TEST_F(AccessLogImplTest, requestTracing) {
   InstancePtr log = InstanceImpl::fromJson(loader, api_, dispatcher_, lock_, store, runtime);
 
   {
-    HeaderMapImpl traceable{{"x-request-id", traceable_guid}};
+    HeaderMapImpl forced_header{{"x-request-id", force_tracing_guid}};
     EXPECT_CALL(*file_, write(_));
-    EXPECT_CALL(runtime.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
-        .WillOnce(Return(true));
-
-    log->log(&traceable, &response_headers_, request_info_);
+    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(&not_traceable, &response_headers_, request_info_);
   }
+
+  {
+    HeaderMapImpl sampled_header{{"x-request-id", sample_tracing_guid}};
+    EXPECT_CALL(*file_, write(_)).Times(0);
+    log->log(&sampled_header, &response_headers_, request_info_);
+  }
 }
 
 TEST(AccessLogImplTestCtor, OperatorIsNotSupported) {

test/common/http/conn_manager_utility_test.cc

@@ -97,16 +97,21 @@ 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-9307-8002-969403baf480", headers.get(Headers::get().RequestId));
+
+    EXPECT_EQ("f4dca0a9-12c7-a307-8002-969403baf480", headers.get(Headers::get().RequestId));
   }
 
   {
     // Not internal request, force trace header should be cleaned.
     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"},
@@ -165,7 +172,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));
   }
 }
 

test/common/runtime/uuid_util_test.cc

@@ -57,11 +57,20 @@ TEST(UUIDUtilsTest, setAndCheckTraceable) {
   Runtime::RandomGeneratorImpl random;
 
   std::string uuid = random.uuid();
-  EXPECT_FALSE(UuidUtils::isTraceableUuid(uuid));
+  EXPECT_EQ(UuidTraceStatus::NoTrace, UuidUtils::isTraceableUuid(uuid));
 
-  EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid));
-  EXPECT_TRUE(UuidUtils::isTraceableUuid(uuid));
+  EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::Sampled));
+  EXPECT_EQ(UuidTraceStatus::Sampled, UuidUtils::isTraceableUuid(uuid));
+
+  EXPECT_TRUE(UuidUtils::setTraceableUuid(uuid, UuidTraceStatus::Client));
+  EXPECT_EQ(UuidTraceStatus::Client, UuidUtils::isTraceableUuid(uuid));
+
+  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));
+  EXPECT_FALSE(UuidUtils::setTraceableUuid(invalid_uuid, UuidTraceStatus::Forced));
 }