Differentiate PacketDropReasons

Summary: The existing PacketDropReason values cover many branches in the code making it impossible to isolate the reason for a PARSE_ERROR, INVALID_PACKET, CONNECTION_NOT_FOUND. This change breaks them down into more values that are each used in a single branch.

Reviewed By: mjoras

Differential Revision: D39149490

fbshipit-source-id: 28cbe1ea6c4a06cf55960058edaa48c28ed4d2ef

quic/client/QuicClientTransport.cpp

@@ -268,7 +268,8 @@ void QuicClientTransport::processPacketData(
 
   RegularQuicPacket* regularOptional = parsedPacket.regularPacket();
   if (!regularOptional) {
-    QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+    QUIC_STATS(
+        statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR_CLIENT);
     if (conn_->qLogger) {
       conn_->qLogger->addPacketDrop(packetSize, kParse);
     }

quic/fizz/client/test/QuicClientTransportTest.cpp

@@ -2765,7 +2765,7 @@ TEST_F(
 
 TEST_P(QuicClientTransportAfterStartTest, ReadStreamCoalesced) {
   expectQuicStatsPacketDrop(
-      QuicTransportStatsCallback::PacketDropReason::PARSE_ERROR);
+      QuicTransportStatsCallback::PacketDropReason::PARSE_ERROR_CLIENT);
   uint8_t connIdSize = GetParam();
 
   client->getNonConstConn().clientConnectionId =
@@ -5465,7 +5465,7 @@ INSTANTIATE_TEST_SUITE_P(
 
 TEST_F(QuicProcessDataTest, ProcessDataWithGarbageAtEnd) {
   expectQuicStatsPacketDrop(
-      QuicTransportStatsCallback::PacketDropReason::PARSE_ERROR);
+      QuicTransportStatsCallback::PacketDropReason::PARSE_ERROR_CLIENT);
   auto qLogger = std::make_shared<FileQLogger>(VantagePoint::Client);
   client->getNonConstConn().qLogger = qLogger;
   auto params = mockClientHandshake->getServerTransportParams();

quic/server/QuicServerWorker.cpp

@@ -214,7 +214,7 @@ bool QuicServerWorker::maybeSendVersionNegotiationPacketOrDrop(
   if (isInitial && datagramLen < kMinInitialPacketSize) {
     VLOG(3) << "Dropping initial packet due to invalid size";
     QUIC_STATS(
-        statsCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET);
+        statsCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET_SIZE);
     return true;
   }
   isInitial =
@@ -235,7 +235,9 @@ bool QuicServerWorker::maybeSendVersionNegotiationPacketOrDrop(
     if (negotiationNeeded && !isInitial) {
       VLOG(3) << "Dropping non-initial packet due to invalid version";
       QUIC_STATS(
-          statsCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET);
+          statsCallback_,
+          onPacketDropped,
+          PacketDropReason::INVALID_PACKET_VERSION);
       return true;
     }
     if (negotiationNeeded) {
@@ -387,7 +389,9 @@ void QuicServerWorker::handleNetworkData(
     if (!cursor.canAdvance(sizeof(uint8_t))) {
       VLOG(4) << "Dropping packet too small";
       QUIC_STATS(
-          statsCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET);
+          statsCallback_,
+          onPacketDropped,
+          PacketDropReason::INVALID_PACKET_INITIAL_BYTE);
       return;
     }
     uint8_t initialByte = cursor.readBE<uint8_t>();
@@ -399,7 +403,9 @@ void QuicServerWorker::handleNetworkData(
       if (!parsedShortHeader) {
         if (!tryHandlingAsHealthCheck(client, *data)) {
           QUIC_STATS(
-              statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+              statsCallback_,
+              onPacketDropped,
+              PacketDropReason::PARSE_ERROR_SHORT_HEADER);
           VLOG(6) << "Failed to parse short header";
         }
         return;
@@ -424,7 +430,9 @@ void QuicServerWorker::handleNetworkData(
     if (!parsedLongHeader) {
       if (!tryHandlingAsHealthCheck(client, *data)) {
         QUIC_STATS(
-            statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+            statsCallback_,
+            onPacketDropped,
+            PacketDropReason::PARSE_ERROR_LONG_HEADER);
         VLOG(6) << "Failed to parse long header";
       }
       return;
@@ -458,7 +466,9 @@ void QuicServerWorker::handleNetworkData(
       // drop packet if connId is present but is not valid.
       VLOG(3) << "Dropping packet due to invalid connectionId";
       QUIC_STATS(
-          statsCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET);
+          statsCallback_,
+          onPacketDropped,
+          PacketDropReason::INVALID_PACKET_CID);
       return;
     }
     RoutingData routingData(
@@ -476,7 +486,10 @@ void QuicServerWorker::handleNetworkData(
         isForwardedData);
   } catch (const std::exception& ex) {
     // Drop the packet.
-    QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+    QUIC_STATS(
+        statsCallback_,
+        onPacketDropped,
+        PacketDropReason::PARSE_ERROR_EXCEPTION);
     VLOG(6) << "Failed to parse packet header " << ex.what();
   }
 }
@@ -602,7 +615,7 @@ void QuicServerWorker::forwardNetworkData(
       QUIC_STATS(
           statsCallback_,
           onPacketDropped,
-          PacketDropReason::CONNECTION_NOT_FOUND);
+          PacketDropReason::UNKNOWN_CID_VERSION);
     }
     return;
   }
@@ -693,7 +706,7 @@ void QuicServerWorker::dispatchPacketData(
           QUIC_STATS(
               statsCallback_,
               onPacketDropped,
-              PacketDropReason::INVALID_PACKET);
+              PacketDropReason::INVALID_PACKET_SIZE_INITIAL);
           return;
         }
 
@@ -868,7 +881,10 @@ void QuicServerWorker::dispatchPacketData(
   if (!connIdAlgo_->canParse(routingData.destinationConnId)) {
     VLOG(3) << "Dropping packet with bad DCID, routingInfo="
             << logRoutingInfo(routingData.destinationConnId);
-    QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+    QUIC_STATS(
+        statsCallback_,
+        onPacketDropped,
+        PacketDropReason::PARSE_ERROR_BAD_DCID);
     // TODO do we need to reset?
     return;
   }
@@ -880,7 +896,8 @@ void QuicServerWorker::dispatchPacketData(
         connIdParam.error().what(),
         folly::to<std::string>(connIdParam.error().errorCode()),
         logRoutingInfo(routingData.destinationConnId));
-    QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+    QUIC_STATS(
+        statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR_DCID);
     // TODO do we need to reset?
     return;
   }
@@ -905,9 +922,7 @@ void QuicServerWorker::dispatchPacketData(
 
   if (!packetForwardingEnabled_ || isForwardedData) {
     QUIC_STATS(
-        statsCallback_,
+        statsCallback_, onPacketDropped, PacketDropReason::CANNOT_FORWARD_DATA);
-        onPacketDropped,
-        PacketDropReason::CONNECTION_NOT_FOUND);
     return sendResetPacket(
         routingData.headerForm,
         client,

quic/server/state/ServerStateMachine.cpp

@@ -692,6 +692,8 @@ void handleCipherUnavailable(
     if (conn.qLogger) {
       conn.qLogger->addPacketDrop(packetSize, kNoData);
     }
+    QUIC_STATS(
+        conn.statsCallback, onPacketDropped, PacketDropReason::EMPTY_DATA);
     return;
   }
   if (originalData->protectionType != ProtectionType::ZeroRtt &&
@@ -700,6 +702,10 @@ void handleCipherUnavailable(
     if (conn.qLogger) {
       conn.qLogger->addPacketDrop(packetSize, kUnexpectedProtectionLevel);
     }
+    QUIC_STATS(
+        conn.statsCallback,
+        onPacketDropped,
+        PacketDropReason::UNEXPECTED_PROTECTION_LEVEL);
     return;
   }
 
@@ -711,6 +717,8 @@ void handleCipherUnavailable(
     if (conn.qLogger) {
       conn.qLogger->addPacketDrop(packetSize, kMaxBuffered);
     }
+    QUIC_STATS(
+        conn.statsCallback, onPacketDropped, PacketDropReason::MAX_BUFFERED);
     return;
   }
 
@@ -721,6 +729,10 @@ void handleCipherUnavailable(
     if (conn.qLogger) {
       conn.qLogger->addPacketBuffered(originalData->protectionType, packetSize);
     }
+    QUIC_STATS(
+        conn.statsCallback,
+        onPacketDropped,
+        PacketDropReason::PARSE_ERROR_PACKET_BUFFERED);
     ServerEvents::ReadData pendingReadData;
     pendingReadData.peer = readData.peer;
     pendingReadData.networkData = NetworkDataSingle(
@@ -735,6 +747,11 @@ void handleCipherUnavailable(
     if (conn.qLogger) {
       conn.qLogger->addPacketDrop(packetSize, kBufferUnavailable);
     }
+    QUIC_STATS(
+        conn.statsCallback,
+        onPacketDropped,
+        PacketDropReason::BUFFER_UNAVAILABLE);
+    return;
   }
 }
 
@@ -760,10 +777,12 @@ void onServerReadDataFromOpen(
         conn.qLogger->addPacketDrop(
             0,
             QuicTransportStatsCallback::toString(
-                PacketDropReason::PARSE_ERROR));
+                PacketDropReason::PARSE_ERROR_LONG_HEADER_INITIAL));
       }
       QUIC_STATS(
-          conn.statsCallback, onPacketDropped, PacketDropReason::PARSE_ERROR);
+          conn.statsCallback,
+          onPacketDropped,
+          PacketDropReason::PARSE_ERROR_LONG_HEADER_INITIAL);
       return;
     }
     QuicVersion version = parsedLongHeader->invariant.version;
@@ -773,12 +792,12 @@ void onServerReadDataFromOpen(
         conn.qLogger->addPacketDrop(
             0,
             QuicTransportStatsCallback::toString(
-                PacketDropReason::INVALID_PACKET));
+                PacketDropReason::INVALID_PACKET_VN));
       }
       QUIC_STATS(
           conn.statsCallback,
           onPacketDropped,
-          PacketDropReason::INVALID_PACKET);
+          PacketDropReason::INVALID_PACKET_VN);
       return;
     }
 
@@ -874,6 +893,10 @@ void onServerReadDataFromOpen(
         if (conn.qLogger) {
           conn.qLogger->addPacketDrop(packetSize, kRetry);
         }
+        QUIC_STATS(
+            conn.statsCallback,
+            onPacketDropped,
+            PacketDropReason::UNEXPECTED_RETRY);
         break;
       }
       case CodecResult::Type::STATELESS_RESET: {
@@ -881,6 +904,10 @@ void onServerReadDataFromOpen(
         if (conn.qLogger) {
           conn.qLogger->addPacketDrop(packetSize, kReset);
         }
+        QUIC_STATS(
+            conn.statsCallback,
+            onPacketDropped,
+            PacketDropReason::UNEXPECTED_RESET);
         break;
       }
       case CodecResult::Type::NOTHING: {
@@ -888,6 +915,10 @@ void onServerReadDataFromOpen(
         if (conn.qLogger) {
           conn.qLogger->addPacketDrop(packetSize, kCipherUnavailable);
         }
+        QUIC_STATS(
+            conn.statsCallback,
+            onPacketDropped,
+            PacketDropReason::UNEXPECTED_NOTHING);
         if (firstPacketFromPeer) {
           throw QuicInternalException(
               "Failed to decrypt first packet from peer",
@@ -903,10 +934,8 @@ void onServerReadDataFromOpen(
     if (!regularOptional) {
       // We were unable to parse the packet, drop for now. All the drop reasons
       // should have already been logged into QLogger and QuicTrace inside the
-      // previous switch-case block. We just need to update QUIC_STATS here.
+      // previous switch-case block. All stats have already been updated.
       VLOG(10) << "Not able to parse QUIC packet " << conn;
-      QUIC_STATS(
-          conn.statsCallback, onPacketDropped, PacketDropReason::PARSE_ERROR);
       continue;
     }
     if (regularOptional->frames.empty()) {
@@ -1420,6 +1449,10 @@ void onServerReadDataFromClosed(
       if (conn.qLogger) {
         conn.qLogger->addPacketDrop(packetSize, kCipherUnavailable);
       }
+      QUIC_STATS(
+          conn.statsCallback,
+          onPacketDropped,
+          PacketDropReason::CIPHER_UNAVAILABLE);
       break;
     }
     case CodecResult::Type::RETRY: {
@@ -1428,6 +1461,10 @@ void onServerReadDataFromClosed(
       if (conn.qLogger) {
         conn.qLogger->addPacketDrop(packetSize, kRetry);
       }
+      QUIC_STATS(
+          conn.statsCallback,
+          onPacketDropped,
+          PacketDropReason::UNEXPECTED_RETRY);
       break;
     }
     case CodecResult::Type::STATELESS_RESET: {
@@ -1435,6 +1472,10 @@ void onServerReadDataFromClosed(
       if (conn.qLogger) {
         conn.qLogger->addPacketDrop(packetSize, kReset);
       }
+      QUIC_STATS(
+          conn.statsCallback,
+          onPacketDropped,
+          PacketDropReason::UNEXPECTED_RESET);
       break;
     }
     case CodecResult::Type::NOTHING: {
@@ -1442,6 +1483,10 @@ void onServerReadDataFromClosed(
       if (conn.qLogger) {
         conn.qLogger->addPacketDrop(packetSize, kCipherUnavailable);
       }
+      QUIC_STATS(
+          conn.statsCallback,
+          onPacketDropped,
+          PacketDropReason::UNEXPECTED_NOTHING);
       break;
     }
     case CodecResult::Type::REGULAR_PACKET:
@@ -1450,14 +1495,8 @@ void onServerReadDataFromClosed(
   auto regularOptional = parsedPacket.regularPacket();
   if (!regularOptional) {
     // We were unable to parse the packet, drop for now.
+    // Packet drop has already been added to qlog and stats
     VLOG(10) << "Not able to parse QUIC packet " << conn;
-    if (conn.qLogger) {
-      conn.qLogger->addPacketDrop(
-          packetSize,
-          QuicTransportStatsCallback::toString(PacketDropReason::PARSE_ERROR));
-    }
-    QUIC_STATS(
-        conn.statsCallback, onPacketDropped, PacketDropReason::PARSE_ERROR);
     return;
   }
   if (regularOptional->frames.empty()) {

quic/server/test/QuicServerTest.cpp

@@ -539,7 +539,7 @@ TEST_F(QuicServerWorkerTest, NoConnFoundTestReset) {
       std::move(data),
       getTestConnectionId(hostId_),
       std::move(shortHeaderConnId),
-      QuicTransportStatsCallback::PacketDropReason::CONNECTION_NOT_FOUND);
+      QuicTransportStatsCallback::PacketDropReason::CANNOT_FORWARD_DATA);
 }
 
 TEST_F(QuicServerWorkerTest, RateLimit) {

quic/state/QuicTransportStatsCallback.h

@@ -31,8 +31,28 @@ class QuicTransportStatsCallback {
     NONE,
     CONNECTION_NOT_FOUND,
     DECRYPTION_ERROR,
-    INVALID_PACKET,
+    INVALID_PACKET_SIZE,
-    PARSE_ERROR,
+    INVALID_PACKET_SIZE_INITIAL,
+    INVALID_PACKET_VERSION,
+    INVALID_PACKET_INITIAL_BYTE,
+    INVALID_PACKET_CID,
+    INVALID_PACKET_VN,
+    PARSE_ERROR_SHORT_HEADER,
+    PARSE_ERROR_LONG_HEADER,
+    PARSE_ERROR_LONG_HEADER_INITIAL,
+    PARSE_ERROR_EXCEPTION,
+    PARSE_ERROR_BAD_DCID,
+    PARSE_ERROR_DCID,
+    PARSE_ERROR_PACKET_BUFFERED,
+    PARSE_ERROR_CLIENT,
+    CIPHER_UNAVAILABLE,
+    UNEXPECTED_RETRY,
+    UNEXPECTED_RESET,
+    UNEXPECTED_NOTHING,
+    UNEXPECTED_PROTECTION_LEVEL,
+    EMPTY_DATA,
+    MAX_BUFFERED,
+    BUFFER_UNAVAILABLE,
     PEER_ADDRESS_CHANGE,
     PROTOCOL_VIOLATION,
     ROUTING_ERROR_WRONG_HOST,
@@ -46,6 +66,8 @@ class QuicTransportStatsCallback {
     CLIENT_STATE_CLOSED,
     CLIENT_SHUTDOWN,
     INVALID_SRC_PORT,
+    UNKNOWN_CID_VERSION,
+    CANNOT_FORWARD_DATA,
     // NOTE: MAX should always be at the end
     MAX
   };
@@ -187,10 +209,50 @@ class QuicTransportStatsCallback {
         return "CONNECTION_NOT_FOUND";
       case PacketDropReason::DECRYPTION_ERROR:
         return "DECRYPTION_ERROR";
-      case PacketDropReason::INVALID_PACKET:
+      case PacketDropReason::INVALID_PACKET_SIZE:
-        return "INVALID_PACKET";
+        return "INVALID_PACKET_SIZE";
-      case PacketDropReason::PARSE_ERROR:
+      case PacketDropReason::INVALID_PACKET_SIZE_INITIAL:
-        return "PARSE_ERROR";
+        return "INVALID_PACKET_SIZE_INITIAL";
+      case PacketDropReason::INVALID_PACKET_VERSION:
+        return "INVALID_PACKET_VERSION";
+      case PacketDropReason::INVALID_PACKET_INITIAL_BYTE:
+        return "INVALID_PACKET_INITIAL_BYTE";
+      case PacketDropReason::INVALID_PACKET_CID:
+        return "INVALID_PACKET_CID";
+      case PacketDropReason::INVALID_PACKET_VN:
+        return "INVALID_PACKET_VN";
+      case PacketDropReason::PARSE_ERROR_SHORT_HEADER:
+        return "PARSE_ERROR_SHORT_HEADER";
+      case PacketDropReason::PARSE_ERROR_LONG_HEADER:
+        return "PARSE_ERROR_LONG_HEADER";
+      case PacketDropReason::PARSE_ERROR_EXCEPTION:
+        return "PARSE_ERROR_EXCEPTION";
+      case PacketDropReason::PARSE_ERROR_BAD_DCID:
+        return "PARSE_ERROR_BAD_DCID";
+      case PacketDropReason::PARSE_ERROR_DCID:
+        return "PARSE_ERROR_DCID";
+      case PacketDropReason::PARSE_ERROR_PACKET_BUFFERED:
+        return "PARSE_ERROR_PACKET_BUFFERED";
+      case PacketDropReason::PARSE_ERROR_CLIENT:
+        return "PARSE_ERROR_CLIENT";
+      case PacketDropReason::PARSE_ERROR_LONG_HEADER_INITIAL:
+        return "PARSE_ERROR_LONG_HEADER_INITIAL";
+      case PacketDropReason::CIPHER_UNAVAILABLE:
+        return "CIPHER_UNAVAILABLE";
+      case PacketDropReason::UNEXPECTED_RETRY:
+        return "UNEXPECTED_RETRY";
+      case PacketDropReason::UNEXPECTED_RESET:
+        return "UNEXPECTED_RESET";
+      case PacketDropReason::UNEXPECTED_NOTHING:
+        return "UNEXPECTED_NOTHING";
+      case PacketDropReason::UNEXPECTED_PROTECTION_LEVEL:
+        return "UNEXPECTED_PROTECTION_LEVEL";
+      case PacketDropReason::EMPTY_DATA:
+        return "EMPTY_DATA";
+      case PacketDropReason::MAX_BUFFERED:
+        return "MAX_BUFFERED";
+      case PacketDropReason::BUFFER_UNAVAILABLE:
+        return "BUFFER_UNAVAILABLE";
       case PacketDropReason::PEER_ADDRESS_CHANGE:
         return "PEER_ADDRESS_CHANGE";
       case PacketDropReason::PROTOCOL_VIOLATION:
@@ -217,10 +279,12 @@ class QuicTransportStatsCallback {
         return "CLIENT_SHUTDOWN";
       case PacketDropReason::INVALID_SRC_PORT:
         return "INVALID_SRC_PORT";
+      case PacketDropReason::UNKNOWN_CID_VERSION:
+        return "UNKNOWN_CID_VERSION";
+      case PacketDropReason::CANNOT_FORWARD_DATA:
+        return "CANNOT_FORWARD_DATA";
       case PacketDropReason::MAX:
         return "MAX";
-      default:
-        throw std::runtime_error("Undefined PacketDropReason passed");
     }
   }