diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index 5b84930ff7..ab235ec5af 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -92,8 +92,10 @@ create_subscription( qos); subscription_topic_stats = std::make_shared< - rclcpp::topic_statistics::SubscriptionTopicStatistics - >(node_topics_interface->get_node_base_interface()->get_name(), publisher); + rclcpp::topic_statistics::SubscriptionTopicStatistics>( + node_topics_interface->get_node_base_interface()->get_name(), + topic_name, + publisher); std::weak_ptr< rclcpp::topic_statistics::SubscriptionTopicStatistics diff --git a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp index 4b9221406f..58996ce32a 100644 --- a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp +++ b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp @@ -71,16 +71,18 @@ class SubscriptionTopicStatistics * measure, and publish topic statistics data. This throws an invalid_argument * if the input publisher is null. * - * \param node_name the name of the node, which created this instance, in order to denote - * topic source + * \param node_name the name of the node which created this instance + * \param topic_name the name of the subscribed topic, used to uniquely + * identify statistics sources for multiple subscriptions in the same node * \param publisher instance constructed by the node in order to publish statistics data. * This class owns the publisher. * \throws std::invalid_argument if publisher pointer is nullptr */ SubscriptionTopicStatistics( const std::string & node_name, + const std::string & topic_name, rclcpp::Publisher::SharedPtr publisher) - : node_name_(node_name), + : node_name_(node_name + topic_name), publisher_(std::move(publisher)) { // TODO(dbbonnie): ros-tooling/aws-roadmap/issues/226, received message age diff --git a/rclcpp/test/rclcpp/topic_statistics/test_subscription_topic_statistics.cpp b/rclcpp/test/rclcpp/topic_statistics/test_subscription_topic_statistics.cpp index ce6887c631..de54bdf8d0 100644 --- a/rclcpp/test/rclcpp/topic_statistics/test_subscription_topic_statistics.cpp +++ b/rclcpp/test/rclcpp/topic_statistics/test_subscription_topic_statistics.cpp @@ -85,8 +85,10 @@ class TestSubscriptionTopicStatistics : public SubscriptionTopicStatistics::SharedPtr publisher) - : SubscriptionTopicStatistics(node_name, publisher) + : SubscriptionTopicStatistics( + node_name, topic_name, publisher) { } @@ -391,6 +393,7 @@ TEST_F(TestSubscriptionTopicStatisticsFixture, test_manual_construction) // Construct a separate instance auto sub_topic_stats = std::make_unique>( empty_subscriber->get_name(), + "/test_topic", topic_stats_publisher); // Expect no data has been collected / no samples received @@ -586,3 +589,81 @@ TEST_F(TestSubscriptionTopicStatisticsFixture, test_receive_stats_include_window } } } +/** + * Test that multiple subscriptions on the same node produce differentiated + * measurement_source_name values in their statistics messages. + * Regression test for #2756. + */ +TEST_F(TestSubscriptionTopicStatisticsFixture, test_multiple_subscriptions_differentiated) +{ + constexpr const char kTopicA[]{"/test_topic_a"}; + constexpr const char kTopicB[]{"/test_topic_b"}; + constexpr const uint64_t kNumStatsMessages{4}; + + auto pub_a = std::make_shared("pub_node_a", kTopicA); + auto pub_b = std::make_shared("pub_node_b", kTopicB); + + auto multi_sub_node = std::make_shared("multi_sub_node"); + + auto options = rclcpp::SubscriptionOptions(); + options.topic_stats_options.state = rclcpp::TopicStatisticsState::Enable; + options.topic_stats_options.publish_period = defaultStatisticsPublishPeriod; + + auto cb = [](Empty::UniquePtr msg) {(void) msg;}; + + auto sub_a = multi_sub_node->create_subscription( + kTopicA, + rclcpp::QoS(rclcpp::KeepAll()), + std::function(cb), + options); + + auto sub_b = multi_sub_node->create_subscription( + kTopicB, + rclcpp::QoS(rclcpp::KeepAll()), + std::function(cb), + options); + + auto stats_listener = + std::make_shared( + "multi_sub_stats_listener", + "/statistics", + kNumStatsMessages); + + rclcpp::executors::SingleThreadedExecutor ex; + ex.add_node(pub_a); + ex.add_node(pub_b); + ex.add_node(multi_sub_node); + ex.add_node(stats_listener); + + ex.spin_until_future_complete(stats_listener->GetFuture(), kTestTimeout); + + const auto received_messages = stats_listener->GetReceivedMessages(); + ASSERT_GE(received_messages.size(), kNumStatsMessages); + + std::set source_names; + for (const auto & msg : received_messages) { + source_names.insert(msg.measurement_source_name); + } + + bool found_topic_a = false; + bool found_topic_b = false; + + for (const auto & name : source_names) { + if (name.find("test_topic_a") != std::string::npos) { + found_topic_a = true; + } + + if (name.find("test_topic_b") != std::string::npos) { + found_topic_b = true; + } + } + + EXPECT_TRUE(found_topic_a) + << "No stats found containing topic_a in source name"; + + EXPECT_TRUE(found_topic_b) + << "No stats found containing topic_b in source name"; + + EXPECT_GT(source_names.size(), 1u) + << "Both subscriptions produced the same measurement_source_name"; +}