Fix topic statistics for IPC subscriptions#3130
Conversation
jayyoung
force-pushed
the
fix-topic-stats-when-using-ipc
branch
from
ce94613 to
7df6693
Compare
|
Could you also add a test that verifies that with IPC enabled the stats handler fires and reports a non-nan age? |
|
@fujitatomoya we'd love to get this merged in before the freeze of rclcpp, I believe on the 20th April. Anything else you would like modified? |
If use_intra_process_comms is enabled, messages are delivered via SubscriptionIntraProcess::execute_impl() which previously never called the topic statistics handler, this causes all statistics to report NaN values. The fix here puts a type-erased StatsHandlerFn in SubscriptionIntraProcess and wires it up from the Subscription via a lambda. Using std::function avoids a circular include chain that would occur if subscription_topic_statistics.hpp were included directly from subscription_intra_process.hpp via publisher.hpp/callback_group.hpp. The source_timestamp is set to the receive time so that message_age reports 0ms rather than an un-initialised value. IPC delivery has little/no expected transport latency, by definition so near-zero age is expected. Fixes ros2#2911 Signed-off-by: jayyoung <jay.young@gmail.com>
…drop now variable. Signed-off-by: jayyoung <jay.young@gmail.com>
Signed-off-by: jayyoung <jay.young@gmail.com>
collected when use_intra_process_comms=true. Signed-off-by: jayyoung <jay.young@gmail.com>
jayyoung
force-pushed
the
fix-topic-stats-when-using-ipc
branch
from
7df6693 to
cdf6632
Compare
Added a little one cdf6632 |
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm, test would be ideal.
|
@fujitatomoya a test was added, anything else to test? |
|
Pulls: #3130
|
Signed-off-by: jayyoung <jay.young@gmail.com>
|
just added 175ce2f which fixes the linting failure @fujitatomoya, thanks! can you re-trigger CI please? |
|
@jmachowinski I'm asking you since you are roughly in our timezone, could you please relaunch CI so we can try to get this in before the freeze? Thanks! |
|
Pulls: #3130
|
|
@tonynajjar I don't remember Munich to be in a different time zone than Bremen ;-P |
|
I was including my colleague in the UK in that sentence 😆 Thank you! |
|
@tonynajjar @jayyoung Just ignore the windows build error for now, we are cleaning up the build failures from the MSVC bump. |
|
Windows rerun - no connext, building up to rclcpp, testing just rclcpp |
|
Thank you for the push everyone! Happy freeze |
5 participants
Continuing on from the discussion here on the original fix: #2913 (comment) which addresses #2911 we are opening this PR at @fujitatomoya request.
Description
The root issue: We discovered that when
use_intra_process_commsis enabled, messages are delivered viaSubscriptionIntraProcess::execute_impl()which previously never called the topic statistics handler. This causes all statistics to reportNaNvalues, making it impossible for us distinguish a healthy IPC subscription from an unhealthy one.This fix: Here we add a type-erased
StatsHandlerFnfunction toSubscriptionIntraProcessand connect it up from theSubscriptionvia a lambda. Using the lambda rather than a pointer toSubscriptionTopicStatisticsavoids a circular include chain we discovered that could occur ifsubscription_topic_statistics.hppwas included directly fromsubscription_intra_process.hpp.In the code, we set
source_timestampto the receive time so thatmessage_agereports0msrather than an un-initialised value. Our reasoning is that: IPC delivery has no or little transport latency, so near-zero age is OK and expected. As discussed in the original fix PR, there is a one-time warning emit to inform users thatmessage_ageis not meaningful for IPC subscriptions.IN summary this PR is a revised implementation of the original PR #2913 by @roman-y-wu, with the following differences:
std::functiontype erasure to avoid the circular include issue we foundsource_timestampto prevent message_age reporting an invalid large value (uninitialised timestamp)From the original PR, we kept the one-time
RCLCPP_WARN_ONCElog message to ensures users are not silently surprised bymessage_agereading ~0ms when using IPC subscriptions.Fixes #2911
Is this user-facing behavior change?
Yeah it is for us, and addresses an issue we find in production. With this fix topic statistics now report valid values for subscriptions using IPC. Previously all statistics were NaN when IPC was enabled, and this behaviour bit us.
Did you use Generative AI?
Not on our side. This PR is a small adaptation of a older fix that already existed: #2913
Additional Information
This fix was first validated as a backport onto ROS 2 Kilted (
rclcpp==29.5.6) and has been running in production at Dexory across a fleet of 126 autonomous warehouse robots for over one month with no issues. Our robots use IPC-enabled lidar pipelines and rely on topic stats to monitor sensor health.message_periodstatistics are now correct and our monitoring correctly detects legitimate sensor failures with this fix.