From 1942b8c442a1a039924f030cd8c72ed4b383b438 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 6 Feb 2025 17:40:30 +0000 Subject: [PATCH 01/14] Adding MRP analytics delegate This is a starting point for MRP events to be sent to some sort of delegate interested. The design is intentionally done in this way to reduce code size within the SDK and is meant for applications such as a controller to registers a delegate for MRP events allowing for it to construct analytics. --- src/messaging/BUILD.gn | 1 + .../ReliableMessageAnalyticsDelegate.h | 56 +++ src/messaging/ReliableMessageMgr.cpp | 51 +++ src/messaging/ReliableMessageMgr.h | 9 + .../tests/TestReliableMessageProtocol.cpp | 324 ++++++++++++++++++ 5 files changed, 441 insertions(+) create mode 100644 src/messaging/ReliableMessageAnalyticsDelegate.h diff --git a/src/messaging/BUILD.gn b/src/messaging/BUILD.gn index a170d625111a06..8e94a2b0d0b5e7 100644 --- a/src/messaging/BUILD.gn +++ b/src/messaging/BUILD.gn @@ -63,6 +63,7 @@ static_library("messaging") { "ExchangeMgr.cpp", "ExchangeMgr.h", "Flags.h", + "ReliableMessageAnalyticsDelegate.h", "ReliableMessageContext.cpp", "ReliableMessageContext.h", "ReliableMessageMgr.cpp", diff --git a/src/messaging/ReliableMessageAnalyticsDelegate.h b/src/messaging/ReliableMessageAnalyticsDelegate.h new file mode 100644 index 00000000000000..49fad476164991 --- /dev/null +++ b/src/messaging/ReliableMessageAnalyticsDelegate.h @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2025 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file defines interface for objects interested in MRP events for analytics + */ + +#pragma once + +#include +#include + +namespace chip { +namespace Messaging { + +class ReliableMessageAnalyticsDelegate +{ +public: + virtual ~ReliableMessageAnalyticsDelegate() = default; + enum class EventType + { + kInitialSend, + kRetransmission, + kAcknowledged, + kFailed, + kUndefined, /* Should be last element in enum */ + }; + + struct TransmitEvent + { + NodeId nodeId = kUndefinedNodeId; + FabricIndex fabricIndex = kUndefinedFabricIndex; + EventType eventType = EventType::kUndefined; + uint32_t messageCounter = 0; + }; + + virtual void OnTransmitEvent(const TransmitEvent & event) = 0; +}; + +} // namespace Messaging +} // namespace chip diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 119adeef876972..6c366106089fe5 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -155,6 +155,17 @@ void ReliableMessageMgr::ExecuteActions() Transport::GetSessionTypeString(session), fabricIndex, ChipLogValueX64(destination), CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS); + if (mAnalyticsDelegate) + { + ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, + .fabricIndex = fabricIndex, + .eventType = + ReliableMessageAnalyticsDelegate::EventType::kFailed, + .messageCounter = messageCounter }; + + mAnalyticsDelegate->OnTransmitEvent(event); + } + // If the exchange is expecting a response, it will handle sending // this notification once it detects that it has not gotten a // response. Otherwise, we need to do it. @@ -295,6 +306,25 @@ bool ReliableMessageMgr::CheckAndRemRetransTable(ReliableMessageContext * rc, ui mRetransTable.ForEachActiveObject([&](auto * entry) { if (entry->ec->GetReliableMessageContext() == rc && entry->retainedBuf.GetMessageCounter() == ackMessageCounter) { + if (mAnalyticsDelegate) + { + auto session = entry->ec->GetSessionHandle(); + auto fabricIndex = session->GetFabricIndex(); + auto destination = kUndefinedNodeId; + if (session->IsSecureSession()) + { + destination = session->AsSecureSession()->GetPeerNodeId(); + } + ReliableMessageAnalyticsDelegate::TransmitEvent event = { + .nodeId = destination, + .fabricIndex = fabricIndex, + .eventType = ReliableMessageAnalyticsDelegate::EventType::kAcknowledged, + .messageCounter = ackMessageCounter + }; + + mAnalyticsDelegate->OnTransmitEvent(event); + } + // Clear the entry from the retransmision table. ClearRetransTable(*entry); @@ -440,6 +470,11 @@ void ReliableMessageMgr::RegisterSessionUpdateDelegate(SessionUpdateDelegate * s mSessionUpdateDelegate = sessionUpdateDelegate; } +void ReliableMessageMgr::RegisterAnalyticsDelegate(ReliableMessageAnalyticsDelegate * analyticsDelegate) +{ + mAnalyticsDelegate = analyticsDelegate; +} + CHIP_ERROR ReliableMessageMgr::MapSendError(CHIP_ERROR error, uint16_t exchangeId, bool isInitiator) { if ( @@ -511,6 +546,22 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) peerIsActive = sessionHandle->AsUnauthenticatedSession()->IsPeerActive(); } + // For initial send the packet has already been submitted to transport layer successfully. + // On re-transmits we do not know if transport layer is unable to retransmit for some + // reason, so saying we have sent re-transmit here is a little presumptuous. + if (mAnalyticsDelegate) + { + ReliableMessageAnalyticsDelegate::TransmitEvent event = { + .nodeId = destination, + .fabricIndex = fabricIndex, + .eventType = entry.sendCount == 0 ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend + : ReliableMessageAnalyticsDelegate::EventType::kRetransmission, + .messageCounter = messageCounter + }; + + mAnalyticsDelegate->OnTransmitEvent(event); + } + ChipLogProgress(ExchangeManager, "??%d [E:" ChipLogFormatExchange " S:%u M:" ChipLogFormatMessageCounter "] (%s) Msg Retransmission to %u:" ChipLogFormatX64 " in %" PRIu32 "ms [State:%s II:%" PRIu32 " AI:%" PRIu32 diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 5036b832108443..2875518f0acd19 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -185,6 +186,13 @@ class ReliableMessageMgr */ void RegisterSessionUpdateDelegate(SessionUpdateDelegate * sessionUpdateDelegate); + /** + * Registers a delegate interested in analytic information + * + * @param[in] analyticsDelegate - Pointer to delegate for reporting analytic + */ + void RegisterAnalyticsDelegate(ReliableMessageAnalyticsDelegate * analyticsDelegate); + /** * Map a send error code to the error code we should actually use for * success checks. This maps some error codes to CHIP_NO_ERROR as @@ -249,6 +257,7 @@ class ReliableMessageMgr ObjectPool mRetransTable; SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; + ReliableMessageAnalyticsDelegate * mAnalyticsDelegate = nullptr; static System::Clock::Timeout sAdditionalMRPBackoffTime; }; diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 6390e4eca1920e..7f1a7be7f15fad 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -21,6 +21,8 @@ * This file implements unit tests for the ReliableMessageProtocol * implementation. */ +#include + #include #include @@ -61,6 +63,13 @@ using namespace chip::System::Clock::Literals; const char PAYLOAD[] = "Hello!"; +class TestReliablityAnalyticDelegate : public ReliableMessageAnalyticsDelegate +{ +public: + virtual void OnTransmitEvent(const TransmitEvent & event) override { mTransmitEvents.push(event); } + std::queue mTransmitEvents; +}; + class TestReliableMessageProtocol : public chip::Test::LoopbackMessagingContext { public: @@ -2043,6 +2052,321 @@ TEST_F(TestReliableMessageProtocol, CheckApplicationResponseNeverComes) EXPECT_EQ(err, CHIP_NO_ERROR); } +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEventualSuccess) +{ + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); + EXPECT_FALSE(buffer.IsNull()); + + CHIP_ERROR err = CHIP_NO_ERROR; + + MockAppDelegate mockSender(*this); + ExchangeContext * exchange = NewExchangeToAlice(&mockSender); + ASSERT_NE(exchange, nullptr); + + ReliableMessageMgr * rm = GetExchangeManager().GetReliableMessageMgr(); + ASSERT_NE(rm, nullptr); + TestReliablityAnalyticDelegate testAnalyticsDelegate; + rm->RegisterAnalyticsDelegate(&testAnalyticsDelegate); + + exchange->GetSessionHandle()->AsSecureSession()->SetRemoteSessionParameters(ReliableMessageProtocolConfig({ + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL + })); + + const auto expectedFabricIndex = exchange->GetSessionHandle()->GetFabricIndex(); + const auto expectedNodeId = exchange->GetSessionHandle()->AsSecureSession()->GetPeerNodeId(); + + // Let's drop the initial message + auto & loopback = GetLoopback(); + loopback.mSentMessageCount = 0; + loopback.mNumMessagesToDrop = 4; + loopback.mDroppedMessageCount = 0; + + // Ensure the retransmit table is empty right now + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); + EXPECT_EQ(err, CHIP_NO_ERROR); + DrainAndServiceIO(); + + // Ensure the initial message was dropped and was added to retransmit table + EXPECT_EQ(loopback.mNumMessagesToDrop, 3u); + EXPECT_EQ(loopback.mDroppedMessageCount, 1u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + // Wait for the initial message to fail (should take 330-413ms) + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 2; }); + DrainAndServiceIO(); + + // Ensure the 1st retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 2u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 2u); + EXPECT_EQ(loopback.mDroppedMessageCount, 2u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 3; }); + DrainAndServiceIO(); + + // Ensure the 2nd retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 3u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 1u); + EXPECT_EQ(loopback.mDroppedMessageCount, 3u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 4; }); + DrainAndServiceIO(); + + // Ensure the 3rd retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 4u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 0u); + EXPECT_EQ(loopback.mDroppedMessageCount, 4u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + // Trigger final transmission + GetIOContext().DriveIOUntil(1500_ms32, [&] { return loopback.mSentMessageCount >= 5; }); + DrainAndServiceIO(); + + // Ensure the last retransmission was NOT dropped, and the retransmit table is empty, as we should have gotten an ack + EXPECT_GE(loopback.mSentMessageCount, 5u); + EXPECT_EQ(loopback.mDroppedMessageCount, 4u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 6u); + auto firstTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(firstTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(firstTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(firstTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); + // We have no way of validating the first messageCounter since this is a randomly generated value, but it should + // remain constant for all subsequent transmit events in this test. + const uint32_t messageCounter = firstTransmitEvent.messageCounter; + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto secondTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(secondTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(secondTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(secondTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, secondTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto thirdTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(thirdTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(thirdTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(thirdTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, thirdTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto forthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(forthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(forthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(forthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, forthTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto fifthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(fifthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(fifthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(fifthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, fifthTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto sixthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(sixthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(sixthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(sixthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); + EXPECT_EQ(messageCounter, sixthTransmitEvent.messageCounter); +} + +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitFailure) +{ + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); + EXPECT_FALSE(buffer.IsNull()); + + CHIP_ERROR err = CHIP_NO_ERROR; + + MockAppDelegate mockSender(*this); + ExchangeContext * exchange = NewExchangeToAlice(&mockSender); + ASSERT_NE(exchange, nullptr); + + ReliableMessageMgr * rm = GetExchangeManager().GetReliableMessageMgr(); + ASSERT_NE(rm, nullptr); + TestReliablityAnalyticDelegate testAnalyticsDelegate; + rm->RegisterAnalyticsDelegate(&testAnalyticsDelegate); + + exchange->GetSessionHandle()->AsSecureSession()->SetRemoteSessionParameters(ReliableMessageProtocolConfig({ + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL + })); + + const auto expectedFabricIndex = exchange->GetSessionHandle()->GetFabricIndex(); + const auto expectedNodeId = exchange->GetSessionHandle()->AsSecureSession()->GetPeerNodeId(); + + // Let's drop the initial message + auto & loopback = GetLoopback(); + loopback.mSentMessageCount = 0; + loopback.mNumMessagesToDrop = 5; + loopback.mDroppedMessageCount = 0; + + // Ensure the retransmit table is empty right now + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); + EXPECT_EQ(err, CHIP_NO_ERROR); + DrainAndServiceIO(); + + // Ensure the initial message was dropped and was added to retransmit table + EXPECT_EQ(loopback.mNumMessagesToDrop, 4u); + EXPECT_EQ(loopback.mDroppedMessageCount, 1u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + // Wait for the initial message to fail (should take 330-413ms) + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 2; }); + DrainAndServiceIO(); + + // Ensure the 1st retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 2u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 3u); + EXPECT_EQ(loopback.mDroppedMessageCount, 2u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 3; }); + DrainAndServiceIO(); + + // Ensure the 2nd retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 3u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 2u); + EXPECT_EQ(loopback.mDroppedMessageCount, 3u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 4; }); + DrainAndServiceIO(); + + // Ensure the 3rd retry was dropped, and is still there in the retransmit table + EXPECT_EQ(loopback.mSentMessageCount, 4u); + EXPECT_EQ(loopback.mNumMessagesToDrop, 1u); + EXPECT_EQ(loopback.mDroppedMessageCount, 4u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + + // Trigger final transmission + GetIOContext().DriveIOUntil(1500_ms32, [&] { return loopback.mSentMessageCount >= 5; }); + DrainAndServiceIO(); + + // Ensure the last retransmission was NOT dropped, and the retransmit table is empty, as we should have gotten an ack + EXPECT_GE(loopback.mSentMessageCount, 5u); + EXPECT_EQ(rm->TestGetCountRetransTable(), 1); + EXPECT_EQ(loopback.mDroppedMessageCount, 5u); + + // Now wait for our exchange to time out. + GetIOContext().DriveIOUntil(3000_ms32, [&] { return rm->TestGetCountRetransTable() == 0; }); + DrainAndServiceIO(); + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 6u); + auto firstTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(firstTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(firstTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(firstTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); + // We have no way of validating the first messageCounter since this is a randomly generated value, but it should + // remain constant for all subsequent transmit events in this test. + const uint32_t messageCounter = firstTransmitEvent.messageCounter; + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto secondTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(secondTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(secondTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(secondTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, secondTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto thirdTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(thirdTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(thirdTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(thirdTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, thirdTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto forthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(forthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(forthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(forthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, forthTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto fifthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(fifthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(fifthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(fifthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kRetransmission); + EXPECT_EQ(messageCounter, fifthTransmitEvent.messageCounter); + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto sixthTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(sixthTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(sixthTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(sixthTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kFailed); + EXPECT_EQ(messageCounter, sixthTransmitEvent.messageCounter); +} + +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitUnauthenticatedExchange) +{ + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); + EXPECT_FALSE(buffer.IsNull()); + + MockSessionEstablishmentDelegate mockReceiver; + CHIP_ERROR err = GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); + EXPECT_EQ(err, CHIP_NO_ERROR); + + MockSessionEstablishmentDelegate mockSender; + ExchangeContext * exchange = NewUnauthenticatedExchangeToAlice(&mockSender); + ASSERT_NE(exchange, nullptr); + + ReliableMessageMgr * rm = GetExchangeManager().GetReliableMessageMgr(); + ASSERT_NE(rm, nullptr); + + TestReliablityAnalyticDelegate testAnalyticsDelegate; + rm->RegisterAnalyticsDelegate(&testAnalyticsDelegate); + + exchange->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(ReliableMessageProtocolConfig({ + 64_ms32, // CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL + 64_ms32, // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL + })); + + const auto expectedFabricIndex = exchange->GetSessionHandle()->GetFabricIndex(); + const auto expectedNodeId = exchange->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerNodeId(); + + auto & loopback = GetLoopback(); + loopback.mSentMessageCount = 0; + loopback.mNumMessagesToDrop = 0; + loopback.mDroppedMessageCount = 0; + + // Ensure the retransmit table is empty right now + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); + EXPECT_EQ(err, CHIP_NO_ERROR); + DrainAndServiceIO(); + + // Test that the message was actually sent (and not dropped) + EXPECT_EQ(loopback.mSentMessageCount, 2u); + EXPECT_EQ(loopback.mDroppedMessageCount, 0u); + + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 2u); + auto firstTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(firstTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(firstTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(firstTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); + // We have no way of validating the first messageCounter since this is a randomly generated value, but it should + // remain constant for all subsequent transmit events in this test. + const uint32_t messageCounter = firstTransmitEvent.messageCounter; + + testAnalyticsDelegate.mTransmitEvents.pop(); + auto secondTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); + EXPECT_EQ(secondTransmitEvent.nodeId, expectedNodeId); + EXPECT_EQ(secondTransmitEvent.fabricIndex, expectedFabricIndex); + EXPECT_EQ(secondTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); + EXPECT_EQ(messageCounter, secondTransmitEvent.messageCounter); +} + /** * TODO: A test that we should have but can't write with the existing * infrastructure we have: From 2c393c939d63cb1716229b42ce22237bce777f90 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 6 Feb 2025 20:52:35 +0000 Subject: [PATCH 02/14] Restyled by clang-format --- src/messaging/ReliableMessageMgr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 2875518f0acd19..0227377eec7f5d 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -256,7 +256,7 @@ class ReliableMessageMgr // ReliableMessageProtocol Global tables for timer context ObjectPool mRetransTable; - SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; + SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; ReliableMessageAnalyticsDelegate * mAnalyticsDelegate = nullptr; static System::Clock::Timeout sAdditionalMRPBackoffTime; From 1c1186cbb22f18949c7936054cc2d294178b3ee3 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 10 Feb 2025 14:50:33 +0000 Subject: [PATCH 03/14] Fix CI and address self comments --- .../ReliableMessageAnalyticsDelegate.h | 2 +- src/messaging/ReliableMessageMgr.cpp | 35 ++++++++++++++----- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/messaging/ReliableMessageAnalyticsDelegate.h b/src/messaging/ReliableMessageAnalyticsDelegate.h index 49fad476164991..a07022b7ee86d7 100644 --- a/src/messaging/ReliableMessageAnalyticsDelegate.h +++ b/src/messaging/ReliableMessageAnalyticsDelegate.h @@ -34,11 +34,11 @@ class ReliableMessageAnalyticsDelegate virtual ~ReliableMessageAnalyticsDelegate() = default; enum class EventType { + kUndefined, kInitialSend, kRetransmission, kAcknowledged, kFailed, - kUndefined, /* Should be last element in enum */ }; struct TransmitEvent diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index ed7a925812fa14..dd7b3b1d4125a1 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -157,6 +157,14 @@ void ReliableMessageMgr::ExecuteActions() if (mAnalyticsDelegate) { +#if !CHIP_PROGRESS_LOGGING + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; + if (sessionHandle->IsSecureSession()) + { + destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + } +#endif // !CHIP_PROGRESS_LOGGING ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, .fabricIndex = fabricIndex, .eventType = @@ -546,11 +554,29 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) peerIsActive = sessionHandle->AsUnauthenticatedSession()->IsPeerActive(); } + ChipLogProgress(ExchangeManager, + "??%d [E:" ChipLogFormatExchange " S:%u M:" ChipLogFormatMessageCounter + "] (%s) Msg Retransmission to %u:" ChipLogFormatX64 " scheduled for %" PRIu32 + "ms from now [State:%s II:%" PRIu32 " AI:%" PRIu32 " AT:%u]", + entry.sendCount + 1, ChipLogValueExchange(&entry.ec.Get()), sessionHandle->SessionIdForLogging(), + messageCounter, Transport::GetSessionTypeString(sessionHandle), fabricIndex, ChipLogValueX64(destination), + backoff.count(), peerIsActive ? "Active" : "Idle", config.mIdleRetransTimeout.count(), + config.mActiveRetransTimeout.count(), config.mActiveThresholdTime.count()); +#endif // CHIP_PROGRESS_LOGGING + // For initial send the packet has already been submitted to transport layer successfully. // On re-transmits we do not know if transport layer is unable to retransmit for some // reason, so saying we have sent re-transmit here is a little presumptuous. if (mAnalyticsDelegate) { +#if !CHIP_PROGRESS_LOGGING + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; + if (sessionHandle->IsSecureSession()) + { + destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + } +#endif // !CHIP_PROGRESS_LOGGING ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, .fabricIndex = fabricIndex, @@ -562,15 +588,6 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) mAnalyticsDelegate->OnTransmitEvent(event); } - ChipLogProgress(ExchangeManager, - "??%d [E:" ChipLogFormatExchange " S:%u M:" ChipLogFormatMessageCounter - "] (%s) Msg Retransmission to %u:" ChipLogFormatX64 " scheduled for %" PRIu32 - "ms from now [State:%s II:%" PRIu32 " AI:%" PRIu32 " AT:%u]", - entry.sendCount + 1, ChipLogValueExchange(&entry.ec.Get()), sessionHandle->SessionIdForLogging(), - messageCounter, Transport::GetSessionTypeString(sessionHandle), fabricIndex, ChipLogValueX64(destination), - backoff.count(), peerIsActive ? "Active" : "Idle", config.mIdleRetransTimeout.count(), - config.mActiveRetransTimeout.count(), config.mActiveThresholdTime.count()); -#endif // CHIP_PROGRESS_LOGGING } #if CHIP_CONFIG_TEST From 98240fa40e55584e455f022c78ec23b4ab2e7b6a Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Mon, 10 Feb 2025 14:51:19 +0000 Subject: [PATCH 04/14] Restyled by clang-format --- src/messaging/ReliableMessageMgr.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index dd7b3b1d4125a1..d8a4727971b931 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -158,11 +158,11 @@ void ReliableMessageMgr::ExecuteActions() if (mAnalyticsDelegate) { #if !CHIP_PROGRESS_LOGGING - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; if (sessionHandle->IsSecureSession()) { - destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); } #endif // !CHIP_PROGRESS_LOGGING ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, @@ -570,11 +570,11 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) if (mAnalyticsDelegate) { #if !CHIP_PROGRESS_LOGGING - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; if (sessionHandle->IsSecureSession()) { - destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); } #endif // !CHIP_PROGRESS_LOGGING ReliableMessageAnalyticsDelegate::TransmitEvent event = { @@ -587,7 +587,6 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) mAnalyticsDelegate->OnTransmitEvent(event); } - } #if CHIP_CONFIG_TEST From 1631815048ee4f7a0c56501c2f7637ba811e73af Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 10 Feb 2025 17:36:37 +0000 Subject: [PATCH 05/14] Fix CI --- src/messaging/ReliableMessageMgr.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index d8a4727971b931..9ca6cffa5394fd 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -157,14 +157,15 @@ void ReliableMessageMgr::ExecuteActions() if (mAnalyticsDelegate) { -#if !CHIP_PROGRESS_LOGGING - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; +#if !(CHIP_ERROR_LOGGING || CHIP_PROGRESS_LOGGING) + uint32_t messageCounter = entry->retainedBuf.GetMessageCounter(); + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; if (sessionHandle->IsSecureSession()) { destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); } -#endif // !CHIP_PROGRESS_LOGGING +#endif // !(CHIP_ERROR_LOGGING || CHIP_PROGRESS_LOGGING) ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, .fabricIndex = fabricIndex, .eventType = @@ -570,8 +571,9 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) if (mAnalyticsDelegate) { #if !CHIP_PROGRESS_LOGGING - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; + uint32_t messageCounter = entry.retainedBuf.GetMessageCounter(); + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; if (sessionHandle->IsSecureSession()) { destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); From f1500fe131c5eb6da3188d6e3bd4ea0268359291 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 10 Feb 2025 18:16:20 +0000 Subject: [PATCH 06/14] Cleanup --- src/messaging/ReliableMessageMgr.cpp | 87 ++++++++++------------------ src/messaging/ReliableMessageMgr.h | 3 + 2 files changed, 32 insertions(+), 58 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 9ca6cffa5394fd..5432c131ae01ca 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -101,6 +101,27 @@ void ReliableMessageMgr::TicklessDebugDumpRetransTable(const char * log) #endif } +void ReliableMessageMgr::TransmitEventAnalyticNotification(const RetransTableEntry & entry, const SessionHandle & sessionHandle, + const ReliableMessageAnalyticsDelegate::EventType & eventType) +{ + if (!mAnalyticsDelegate) + { + return; + } + uint32_t messageCounter = entry.retainedBuf.GetMessageCounter(); + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = kUndefinedNodeId; + if (sessionHandle->IsSecureSession()) + { + destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + } + ReliableMessageAnalyticsDelegate::TransmitEvent event = { + .nodeId = destination, .fabricIndex = fabricIndex, .eventType = eventType, .messageCounter = messageCounter + }; + + mAnalyticsDelegate->OnTransmitEvent(event); +} + void ReliableMessageMgr::ExecuteActions() { System::Clock::Timestamp now = System::SystemClock().GetMonotonicTimestamp(); @@ -155,25 +176,7 @@ void ReliableMessageMgr::ExecuteActions() Transport::GetSessionTypeString(session), fabricIndex, ChipLogValueX64(destination), CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS); - if (mAnalyticsDelegate) - { -#if !(CHIP_ERROR_LOGGING || CHIP_PROGRESS_LOGGING) - uint32_t messageCounter = entry->retainedBuf.GetMessageCounter(); - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; - if (sessionHandle->IsSecureSession()) - { - destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); - } -#endif // !(CHIP_ERROR_LOGGING || CHIP_PROGRESS_LOGGING) - ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, - .fabricIndex = fabricIndex, - .eventType = - ReliableMessageAnalyticsDelegate::EventType::kFailed, - .messageCounter = messageCounter }; - - mAnalyticsDelegate->OnTransmitEvent(event); - } + TransmitEventAnalyticNotification(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kFailed); // If the exchange is expecting a response, it will handle sending // this notification once it detects that it has not gotten a @@ -315,24 +318,8 @@ bool ReliableMessageMgr::CheckAndRemRetransTable(ReliableMessageContext * rc, ui mRetransTable.ForEachActiveObject([&](auto * entry) { if (entry->ec->GetReliableMessageContext() == rc && entry->retainedBuf.GetMessageCounter() == ackMessageCounter) { - if (mAnalyticsDelegate) - { - auto session = entry->ec->GetSessionHandle(); - auto fabricIndex = session->GetFabricIndex(); - auto destination = kUndefinedNodeId; - if (session->IsSecureSession()) - { - destination = session->AsSecureSession()->GetPeerNodeId(); - } - ReliableMessageAnalyticsDelegate::TransmitEvent event = { - .nodeId = destination, - .fabricIndex = fabricIndex, - .eventType = ReliableMessageAnalyticsDelegate::EventType::kAcknowledged, - .messageCounter = ackMessageCounter - }; - - mAnalyticsDelegate->OnTransmitEvent(event); - } + auto session = entry->ec->GetSessionHandle(); + TransmitEventAnalyticNotification(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); // Clear the entry from the retransmision table. ClearRetransTable(*entry); @@ -568,27 +555,11 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) // For initial send the packet has already been submitted to transport layer successfully. // On re-transmits we do not know if transport layer is unable to retransmit for some // reason, so saying we have sent re-transmit here is a little presumptuous. - if (mAnalyticsDelegate) - { -#if !CHIP_PROGRESS_LOGGING - uint32_t messageCounter = entry.retainedBuf.GetMessageCounter(); - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; - if (sessionHandle->IsSecureSession()) - { - destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); - } -#endif // !CHIP_PROGRESS_LOGGING - ReliableMessageAnalyticsDelegate::TransmitEvent event = { - .nodeId = destination, - .fabricIndex = fabricIndex, - .eventType = entry.sendCount == 0 ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend - : ReliableMessageAnalyticsDelegate::EventType::kRetransmission, - .messageCounter = messageCounter - }; - - mAnalyticsDelegate->OnTransmitEvent(event); - } + + ReliableMessageAnalyticsDelegate::EventType eventType = entry.sendCount == 0 + ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend + : ReliableMessageAnalyticsDelegate::EventType::kRetransmission; + TransmitEventAnalyticNotification(entry, sessionHandle, eventType); } #if CHIP_CONFIG_TEST diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 0227377eec7f5d..c8bb61f34c8868 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -253,6 +253,9 @@ class ReliableMessageMgr void TicklessDebugDumpRetransTable(const char * log); + void TransmitEventAnalyticNotification(const RetransTableEntry & entry, const SessionHandle & sessionHandle, + const ReliableMessageAnalyticsDelegate::EventType & eventType); + // ReliableMessageProtocol Global tables for timer context ObjectPool mRetransTable; From 7488e5d4d52c7610a53f39c53bfe547b3232d8d2 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 10 Feb 2025 18:28:59 +0000 Subject: [PATCH 07/14] Small nit fix --- src/messaging/ReliableMessageMgr.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 5432c131ae01ca..b115f9f0b0b6e2 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -555,7 +555,6 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) // For initial send the packet has already been submitted to transport layer successfully. // On re-transmits we do not know if transport layer is unable to retransmit for some // reason, so saying we have sent re-transmit here is a little presumptuous. - ReliableMessageAnalyticsDelegate::EventType eventType = entry.sendCount == 0 ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend : ReliableMessageAnalyticsDelegate::EventType::kRetransmission; From a3a81902ed8bca9c216185499a69c20c791af40c Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Feb 2025 20:06:50 +0000 Subject: [PATCH 08/14] Address PR comments --- src/lib/core/BUILD.gn | 1 + src/lib/core/CHIPConfig.h | 14 +++++++++++++ src/lib/core/core.gni | 2 ++ src/messaging/ReliableMessageMgr.cpp | 20 ++++++++++++++----- src/messaging/ReliableMessageMgr.h | 8 ++++++-- .../tests/TestReliableMessageProtocol.cpp | 2 ++ 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index 906b8a4988aee0..89f85d5015b3df 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -71,6 +71,7 @@ buildconfig_header("chip_buildconfig") { "CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ=${chip_tlv_validate_char_string_on_read}", "CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS=${chip_enable_sending_batch_commands}", "CHIP_CONFIG_TEST_GOOGLETEST=${chip_build_tests_googletest}", + "CHIP_CONFIG_MRP_ANALYTICS_ENABLED=${chip_enable_mrp_analytics}", ] visibility = [ ":chip_config_header" ] diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 8731bc4b3019a7..221f9b3751bcf6 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -1867,6 +1867,20 @@ extern const char CHIP_NON_PRODUCTION_MARKER[]; #define CHIP_CONFIG_TEST_GOOGLETEST 0 #endif // CHIP_CONFIG_TEST_GOOGLETEST +/** + * @def CHIP_CONFIG_MRP_ANALYTICS_ENABLED + * + * @brief + * Enables code for collecting and sending analytic related events for MRP + * + * The purpose of this macro is to prevent compiling code related to MRP analytics + * for devices that are not interested interested to save on flash. + */ + +#ifndef CHIP_CONFIG_MRP_ANALYTICS_ENABLED +#define CHIP_CONFIG_MRP_ANALYTICS_ENABLED 0 +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED + /** * @} */ diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index 6dd71cf64f5a57..ef4161ef63f107 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -126,6 +126,8 @@ declare_args() { chip_enable_sending_batch_commands = current_os == "linux" || current_os == "mac" || current_os == "ios" || current_os == "android" + + chip_enable_mrp_analytics = current_os == "linux" || current_os == "android" } if (chip_target_style == "") { diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index b115f9f0b0b6e2..4675dfdd587ef7 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -101,8 +101,9 @@ void ReliableMessageMgr::TicklessDebugDumpRetransTable(const char * log) #endif } -void ReliableMessageMgr::TransmitEventAnalyticNotification(const RetransTableEntry & entry, const SessionHandle & sessionHandle, - const ReliableMessageAnalyticsDelegate::EventType & eventType) +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED +void ReliableMessageMgr::NotifyMessageSendAnalytics(const RetransTableEntry & entry, const SessionHandle & sessionHandle, + const ReliableMessageAnalyticsDelegate::EventType & eventType) { if (!mAnalyticsDelegate) { @@ -121,6 +122,7 @@ void ReliableMessageMgr::TransmitEventAnalyticNotification(const RetransTableEnt mAnalyticsDelegate->OnTransmitEvent(event); } +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED void ReliableMessageMgr::ExecuteActions() { @@ -176,7 +178,9 @@ void ReliableMessageMgr::ExecuteActions() Transport::GetSessionTypeString(session), fabricIndex, ChipLogValueX64(destination), CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS); - TransmitEventAnalyticNotification(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kFailed); +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED + NotifyMessageSendAnalytics(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kFailed); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED // If the exchange is expecting a response, it will handle sending // this notification once it detects that it has not gotten a @@ -318,8 +322,10 @@ bool ReliableMessageMgr::CheckAndRemRetransTable(ReliableMessageContext * rc, ui mRetransTable.ForEachActiveObject([&](auto * entry) { if (entry->ec->GetReliableMessageContext() == rc && entry->retainedBuf.GetMessageCounter() == ackMessageCounter) { +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED auto session = entry->ec->GetSessionHandle(); - TransmitEventAnalyticNotification(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); + NotifyMessageSendAnalytics(*entry, session, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED // Clear the entry from the retransmision table. ClearRetransTable(*entry); @@ -466,10 +472,12 @@ void ReliableMessageMgr::RegisterSessionUpdateDelegate(SessionUpdateDelegate * s mSessionUpdateDelegate = sessionUpdateDelegate; } +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED void ReliableMessageMgr::RegisterAnalyticsDelegate(ReliableMessageAnalyticsDelegate * analyticsDelegate) { mAnalyticsDelegate = analyticsDelegate; } +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED CHIP_ERROR ReliableMessageMgr::MapSendError(CHIP_ERROR error, uint16_t exchangeId, bool isInitiator) { @@ -552,13 +560,15 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) config.mActiveRetransTimeout.count(), config.mActiveThresholdTime.count()); #endif // CHIP_PROGRESS_LOGGING +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED // For initial send the packet has already been submitted to transport layer successfully. // On re-transmits we do not know if transport layer is unable to retransmit for some // reason, so saying we have sent re-transmit here is a little presumptuous. ReliableMessageAnalyticsDelegate::EventType eventType = entry.sendCount == 0 ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend : ReliableMessageAnalyticsDelegate::EventType::kRetransmission; - TransmitEventAnalyticNotification(entry, sessionHandle, eventType); + NotifyMessageSendAnalytics(entry, sessionHandle, eventType); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED } #if CHIP_CONFIG_TEST diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index c8bb61f34c8868..7834476cf275bd 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -186,12 +186,14 @@ class ReliableMessageMgr */ void RegisterSessionUpdateDelegate(SessionUpdateDelegate * sessionUpdateDelegate); +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED /** * Registers a delegate interested in analytic information * * @param[in] analyticsDelegate - Pointer to delegate for reporting analytic */ void RegisterAnalyticsDelegate(ReliableMessageAnalyticsDelegate * analyticsDelegate); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED /** * Map a send error code to the error code we should actually use for @@ -253,8 +255,10 @@ class ReliableMessageMgr void TicklessDebugDumpRetransTable(const char * log); - void TransmitEventAnalyticNotification(const RetransTableEntry & entry, const SessionHandle & sessionHandle, - const ReliableMessageAnalyticsDelegate::EventType & eventType); +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED + void NotifyMessageSendAnalytics(const RetransTableEntry & entry, const SessionHandle & sessionHandle, + const ReliableMessageAnalyticsDelegate::EventType & eventType); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED // ReliableMessageProtocol Global tables for timer context ObjectPool mRetransTable; diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 7f1a7be7f15fad..82a9065a69caa4 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -2052,6 +2052,7 @@ TEST_F(TestReliableMessageProtocol, CheckApplicationResponseNeverComes) EXPECT_EQ(err, CHIP_NO_ERROR); } +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEventualSuccess) { chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); @@ -2366,6 +2367,7 @@ TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitUnau EXPECT_EQ(secondTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); EXPECT_EQ(messageCounter, secondTransmitEvent.messageCounter); } +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED /** * TODO: A test that we should have but can't write with the existing From 7a5645893f05558ee33d4eea8d262ee1832a0b45 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Feb 2025 21:26:54 +0000 Subject: [PATCH 09/14] Fix CI --- src/messaging/ReliableMessageMgr.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 7834476cf275bd..f872436a9238e0 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -264,7 +264,9 @@ class ReliableMessageMgr ObjectPool mRetransTable; SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED ReliableMessageAnalyticsDelegate * mAnalyticsDelegate = nullptr; +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED static System::Clock::Timeout sAdditionalMRPBackoffTime; }; From cb2b590d7f691453fb026579800602021b36eb12 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 18 Feb 2025 14:29:33 +0000 Subject: [PATCH 10/14] Restyle and address PR comments --- .../ReliableMessageAnalyticsDelegate.h | 22 ++++- src/messaging/ReliableMessageMgr.cpp | 39 +++++--- src/messaging/ReliableMessageMgr.h | 2 +- .../tests/TestReliableMessageProtocol.cpp | 89 ++++++++++++++----- 4 files changed, 109 insertions(+), 43 deletions(-) diff --git a/src/messaging/ReliableMessageAnalyticsDelegate.h b/src/messaging/ReliableMessageAnalyticsDelegate.h index a07022b7ee86d7..9d22c7032b1128 100644 --- a/src/messaging/ReliableMessageAnalyticsDelegate.h +++ b/src/messaging/ReliableMessageAnalyticsDelegate.h @@ -17,7 +17,7 @@ /** * @file - * This file defines interface for objects interested in MRP events for analytics + * This file defines and interface for objects interested in MRP events for analytics */ #pragma once @@ -32,9 +32,16 @@ class ReliableMessageAnalyticsDelegate { public: virtual ~ReliableMessageAnalyticsDelegate() = default; + + enum class SessionType + { + kEstablishedCase, + // Initially starting with only one session type but thinking about future when we expand to allow + // other session types like Establishing a CASE session. + }; + enum class EventType { - kUndefined, kInitialSend, kRetransmission, kAcknowledged, @@ -43,9 +50,16 @@ class ReliableMessageAnalyticsDelegate struct TransmitEvent { - NodeId nodeId = kUndefinedNodeId; + // When the session has a peer node ID, this will be a value other than kUndefinedNodeId. + NodeId nodeId = kUndefinedNodeId; + // When the session has a fabric index, this will be a value other than kUndefinedFabricIndex. FabricIndex fabricIndex = kUndefinedFabricIndex; - EventType eventType = EventType::kUndefined; + // Session type of the transmit analytic event. + SessionType sessionType = SessionType::kEstablishedCase; + // The transmit event type. + EventType eventType = EventType::kInitialSend; + // The outgoing message counter associated with the event. If there is no outgoing message counter + // this value will be 0. uint32_t messageCounter = 0; }; diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 4675dfdd587ef7..128af933153ca6 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -105,21 +105,27 @@ void ReliableMessageMgr::TicklessDebugDumpRetransTable(const char * log) void ReliableMessageMgr::NotifyMessageSendAnalytics(const RetransTableEntry & entry, const SessionHandle & sessionHandle, const ReliableMessageAnalyticsDelegate::EventType & eventType) { - if (!mAnalyticsDelegate) + // For now we only support sending analytics for establish CASE sessions. + if (!mAnalyticsDelegate || !sessionHandle->IsSecureSession()) { return; } - uint32_t messageCounter = entry.retainedBuf.GetMessageCounter(); - auto fabricIndex = sessionHandle->GetFabricIndex(); - auto destination = kUndefinedNodeId; - if (sessionHandle->IsSecureSession()) + + auto secureSession = sessionHandle->AsSecureSession(); + if (!secureSession->IsCASESession()) { - destination = sessionHandle->AsSecureSession()->GetPeerNodeId(); + return; } - ReliableMessageAnalyticsDelegate::TransmitEvent event = { - .nodeId = destination, .fabricIndex = fabricIndex, .eventType = eventType, .messageCounter = messageCounter - }; + uint32_t messageCounter = entry.retainedBuf.GetMessageCounter(); + auto fabricIndex = sessionHandle->GetFabricIndex(); + auto destination = secureSession->GetPeerNodeId(); + ReliableMessageAnalyticsDelegate::TransmitEvent event = { .nodeId = destination, + .fabricIndex = fabricIndex, + .sessionType = + ReliableMessageAnalyticsDelegate::SessionType::kEstablishedCase, + .eventType = eventType, + .messageCounter = messageCounter }; mAnalyticsDelegate->OnTransmitEvent(event); } #endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED @@ -366,6 +372,10 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) #if CHIP_CONFIG_ENABLE_ICD_SERVER app::ICDNotifier::GetInstance().NotifyNetworkActivityNotification(); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED + NotifyMessageSendAnalytics(*entry, entry->ec->GetSessionHandle(), + ReliableMessageAnalyticsDelegate::EventType::kRetransmission); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED #if CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE const ExchangeManager * exchangeMgr = entry->ec->GetExchangeMgr(); // TODO: investigate why in ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange unit test released exchange @@ -563,11 +573,12 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) #if CHIP_CONFIG_MRP_ANALYTICS_ENABLED // For initial send the packet has already been submitted to transport layer successfully. // On re-transmits we do not know if transport layer is unable to retransmit for some - // reason, so saying we have sent re-transmit here is a little presumptuous. - ReliableMessageAnalyticsDelegate::EventType eventType = entry.sendCount == 0 - ? ReliableMessageAnalyticsDelegate::EventType::kInitialSend - : ReliableMessageAnalyticsDelegate::EventType::kRetransmission; - NotifyMessageSendAnalytics(entry, sessionHandle, eventType); + // reason. For this reason we only call NotifyMessageSendAnalytics if it is the inital send + // and send kRetransmission after we know transmission is successful elsewhere. + if (entry.sendCount == 0) + { + NotifyMessageSendAnalytics(entry, sessionHandle, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); + } #endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED } diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index f872436a9238e0..9c27186032bb3c 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -263,7 +263,7 @@ class ReliableMessageMgr // ReliableMessageProtocol Global tables for timer context ObjectPool mRetransTable; - SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; + SessionUpdateDelegate * mSessionUpdateDelegate = nullptr; #if CHIP_CONFIG_MRP_ANALYTICS_ENABLED ReliableMessageAnalyticsDelegate * mAnalyticsDelegate = nullptr; #endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 82a9065a69caa4..f30e763c50cd1b 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -2053,13 +2053,20 @@ TEST_F(TestReliableMessageProtocol, CheckApplicationResponseNeverComes) } #if CHIP_CONFIG_MRP_ANALYTICS_ENABLED -TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEventualSuccess) +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEventualSuccessForEsablishedCase) { + CHIP_ERROR err = CHIP_NO_ERROR; + // Make sure we are using CASE sessions, because there is no defunct-marking for PASE. + ExpireSessionBobToAlice(); + ExpireSessionAliceToBob(); + err = CreateCASESessionBobToAlice(); + EXPECT_EQ(err, CHIP_NO_ERROR); + err = CreateCASESessionAliceToBob(); + EXPECT_EQ(err, CHIP_NO_ERROR); + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); EXPECT_FALSE(buffer.IsNull()); - CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockSender(*this); ExchangeContext * exchange = NewExchangeToAlice(&mockSender); ASSERT_NE(exchange, nullptr); @@ -2177,13 +2184,20 @@ TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEven EXPECT_EQ(messageCounter, sixthTransmitEvent.messageCounter); } -TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitFailure) +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitFailureForEsablishedCase) { + CHIP_ERROR err = CHIP_NO_ERROR; + // Make sure we are using CASE sessions, because there is no defunct-marking for PASE. + ExpireSessionBobToAlice(); + ExpireSessionAliceToBob(); + err = CreateCASESessionBobToAlice(); + EXPECT_EQ(err, CHIP_NO_ERROR); + err = CreateCASESessionAliceToBob(); + EXPECT_EQ(err, CHIP_NO_ERROR); + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); EXPECT_FALSE(buffer.IsNull()); - CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockSender(*this); ExchangeContext * exchange = NewExchangeToAlice(&mockSender); ASSERT_NE(exchange, nullptr); @@ -2306,6 +2320,50 @@ TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitFail EXPECT_EQ(messageCounter, sixthTransmitEvent.messageCounter); } +TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitEsablishedPase) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); + EXPECT_FALSE(buffer.IsNull()); + + MockAppDelegate mockSender(*this); + ExchangeContext * exchange = NewExchangeToAlice(&mockSender); + ASSERT_NE(exchange, nullptr); + + ReliableMessageMgr * rm = GetExchangeManager().GetReliableMessageMgr(); + ASSERT_NE(rm, nullptr); + + TestReliablityAnalyticDelegate testAnalyticsDelegate; + rm->RegisterAnalyticsDelegate(&testAnalyticsDelegate); + + exchange->GetSessionHandle()->AsSecureSession()->SetRemoteSessionParameters(ReliableMessageProtocolConfig({ + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL + 30_ms32, // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL + })); + + ASSERT_TRUE(exchange->GetSessionHandle()->AsSecureSession()->IsPASESession()); + + auto & loopback = GetLoopback(); + loopback.mSentMessageCount = 0; + loopback.mNumMessagesToDrop = 0; + loopback.mDroppedMessageCount = 0; + + // Ensure the retransmit table is empty right now + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); + EXPECT_EQ(err, CHIP_NO_ERROR); + DrainAndServiceIO(); + + // Test that the message was actually sent (and not dropped) + EXPECT_EQ(loopback.mSentMessageCount, 2u); + EXPECT_EQ(loopback.mDroppedMessageCount, 0u); + + EXPECT_EQ(rm->TestGetCountRetransTable(), 0); + + ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 0u); +} + TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitUnauthenticatedExchange) { chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); @@ -2330,9 +2388,6 @@ TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitUnau 64_ms32, // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL })); - const auto expectedFabricIndex = exchange->GetSessionHandle()->GetFabricIndex(); - const auto expectedNodeId = exchange->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerNodeId(); - auto & loopback = GetLoopback(); loopback.mSentMessageCount = 0; loopback.mNumMessagesToDrop = 0; @@ -2351,21 +2406,7 @@ TEST_F(TestReliableMessageProtocol, CheckReliableMessageAnalyticsForTransmitUnau EXPECT_EQ(rm->TestGetCountRetransTable(), 0); - ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 2u); - auto firstTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); - EXPECT_EQ(firstTransmitEvent.nodeId, expectedNodeId); - EXPECT_EQ(firstTransmitEvent.fabricIndex, expectedFabricIndex); - EXPECT_EQ(firstTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); - // We have no way of validating the first messageCounter since this is a randomly generated value, but it should - // remain constant for all subsequent transmit events in this test. - const uint32_t messageCounter = firstTransmitEvent.messageCounter; - - testAnalyticsDelegate.mTransmitEvents.pop(); - auto secondTransmitEvent = testAnalyticsDelegate.mTransmitEvents.front(); - EXPECT_EQ(secondTransmitEvent.nodeId, expectedNodeId); - EXPECT_EQ(secondTransmitEvent.fabricIndex, expectedFabricIndex); - EXPECT_EQ(secondTransmitEvent.eventType, ReliableMessageAnalyticsDelegate::EventType::kAcknowledged); - EXPECT_EQ(messageCounter, secondTransmitEvent.messageCounter); + ASSERT_EQ(testAnalyticsDelegate.mTransmitEvents.size(), 0u); } #endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED From fcce160e68334010cdbcd6ef847f208fd4ccf7e1 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 18 Feb 2025 19:40:32 +0000 Subject: [PATCH 11/14] Self-review edits --- src/messaging/ReliableMessageAnalyticsDelegate.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/messaging/ReliableMessageAnalyticsDelegate.h b/src/messaging/ReliableMessageAnalyticsDelegate.h index 9d22c7032b1128..8be1b450aae8b3 100644 --- a/src/messaging/ReliableMessageAnalyticsDelegate.h +++ b/src/messaging/ReliableMessageAnalyticsDelegate.h @@ -17,7 +17,7 @@ /** * @file - * This file defines and interface for objects interested in MRP events for analytics + * This file defines an interface for objects interested in MRP events for analytics */ #pragma once @@ -36,15 +36,19 @@ class ReliableMessageAnalyticsDelegate enum class SessionType { kEstablishedCase, - // Initially starting with only one session type but thinking about future when we expand to allow - // other session types like Establishing a CASE session. + // Initially, we are starting with only one session type, but we are considering the future when we expand to allow + // other session types, such as establishing a CASE session. }; enum class EventType { + // Event associated with first time this specific message is sent. kInitialSend, + // Event associated with re-transmitting a message that was previously sent but not acknowledged. kRetransmission, + // Event associated with receiving an acknowledgement of a previously sent message. kAcknowledged, + // Event associated with transmission of a message that failed to be acknowledged. kFailed, }; From 1607becb3536a8d9152d0c3371516095a6c42216 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 19 Feb 2025 08:21:59 -0500 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/lib/core/core.gni | 2 +- src/messaging/ReliableMessageAnalyticsDelegate.h | 2 +- src/messaging/ReliableMessageMgr.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index ef4161ef63f107..8fcb2c6738344b 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -127,7 +127,7 @@ declare_args() { current_os == "linux" || current_os == "mac" || current_os == "ios" || current_os == "android" - chip_enable_mrp_analytics = current_os == "linux" || current_os == "android" + chip_enable_mrp_analytics = current_os == "linux" || current_os == "android" || current_os == "mac" || current_os == "ios" } if (chip_target_style == "") { diff --git a/src/messaging/ReliableMessageAnalyticsDelegate.h b/src/messaging/ReliableMessageAnalyticsDelegate.h index 8be1b450aae8b3..4eeea4f23b7e5f 100644 --- a/src/messaging/ReliableMessageAnalyticsDelegate.h +++ b/src/messaging/ReliableMessageAnalyticsDelegate.h @@ -58,7 +58,7 @@ class ReliableMessageAnalyticsDelegate NodeId nodeId = kUndefinedNodeId; // When the session has a fabric index, this will be a value other than kUndefinedFabricIndex. FabricIndex fabricIndex = kUndefinedFabricIndex; - // Session type of the transmit analytic event. + // Session type of session the message involved is being sent on. SessionType sessionType = SessionType::kEstablishedCase; // The transmit event type. EventType eventType = EventType::kInitialSend; diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 128af933153ca6..84697dc4d78cbc 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -105,7 +105,7 @@ void ReliableMessageMgr::TicklessDebugDumpRetransTable(const char * log) void ReliableMessageMgr::NotifyMessageSendAnalytics(const RetransTableEntry & entry, const SessionHandle & sessionHandle, const ReliableMessageAnalyticsDelegate::EventType & eventType) { - // For now we only support sending analytics for establish CASE sessions. + // For now we only support sending analytics for messages being sent over an established CASE session. if (!mAnalyticsDelegate || !sessionHandle->IsSecureSession()) { return; From 956f8ed52a1d4027fd69aca21912c7ee062649d0 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 19 Feb 2025 13:27:31 +0000 Subject: [PATCH 13/14] Address PR comment --- src/messaging/ReliableMessageMgr.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 84697dc4d78cbc..9a2bb31777c7e6 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -319,6 +319,9 @@ System::Clock::Timeout ReliableMessageMgr::GetBackoff(System::Clock::Timeout bas void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry) { CalculateNextRetransTime(*entry); +#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED + NotifyMessageSendAnalytics(*entry, entry->ec->GetSessionHandle(), ReliableMessageAnalyticsDelegate::EventType::kInitialSend); +#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED StartTimer(); } @@ -569,17 +572,6 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry) backoff.count(), peerIsActive ? "Active" : "Idle", config.mIdleRetransTimeout.count(), config.mActiveRetransTimeout.count(), config.mActiveThresholdTime.count()); #endif // CHIP_PROGRESS_LOGGING - -#if CHIP_CONFIG_MRP_ANALYTICS_ENABLED - // For initial send the packet has already been submitted to transport layer successfully. - // On re-transmits we do not know if transport layer is unable to retransmit for some - // reason. For this reason we only call NotifyMessageSendAnalytics if it is the inital send - // and send kRetransmission after we know transmission is successful elsewhere. - if (entry.sendCount == 0) - { - NotifyMessageSendAnalytics(entry, sessionHandle, ReliableMessageAnalyticsDelegate::EventType::kInitialSend); - } -#endif // CHIP_CONFIG_MRP_ANALYTICS_ENABLED } #if CHIP_CONFIG_TEST From 42001771500cd5c296c557f369d90e2500c144db Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 19 Feb 2025 13:28:16 +0000 Subject: [PATCH 14/14] Restyled by gn --- src/lib/core/core.gni | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index 8fcb2c6738344b..1f41ed8e6f053e 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -127,7 +127,9 @@ declare_args() { current_os == "linux" || current_os == "mac" || current_os == "ios" || current_os == "android" - chip_enable_mrp_analytics = current_os == "linux" || current_os == "android" || current_os == "mac" || current_os == "ios" + chip_enable_mrp_analytics = + current_os == "linux" || current_os == "android" || current_os == "mac" || + current_os == "ios" } if (chip_target_style == "") {