feat: Added new statistics interface / removed statistics publisher - #3203
jmachowinski wants to merge 2 commits into
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
|
Pulls: #3203 |
|
This reduced the compile time of a node with 1 sub and 1 publisher from 6.32s to 4.91s on a Ryzen 9900X |
fujitatomoya
left a comment
There was a problem hiding this comment.
I am not against this approach, imo old implementation clearly wasn't pulling its weight anyway. a few comments below.
- should we not give deprecation period before removal? statistics has been there for a long time, there would be some users who might use the statistics.
- no default implementation provided by mainline? i think user can still do enable/disable the statistics feature on the subscription, and then mainline provides the default collector?
- this still supports rclcpp-only, so rclpy and other client libraries can't benefit. we know that this is the same limitation the old design had. probably this is next step after this PR, for longer term I think that statistics hooks is an
rcl-level event/emission API, which would give all client libraries and all entity types (publishers, services, actions) a shared mechanism, with a default implementation maintained and tested in mainline.
|
Note, we talked about this on roscon, and we came to the conclusion, that we would remove this completely and not even provide an interface for collecting statistics. |
This removes the half backed statistics implementation and replaces it with a interface that can be implemented by a third party pkg to archive the same functionality. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
3072937 to
861c0dc
Compare
|
Pulls: #3203 |
ABI Compliance Check❌ Verdict: incompatible
❌
|
|
I think that we should probably also make a discourse post about this after we remove it. I don't think anyone will be impacted, but better to be overly-communicative. |
|
Pulls: #3203, ros2/demos#812, ros2/ros2#1878 |
Description
This removes the half backed statistics implementation and replaces it with a interface that can be implemented by a third party pkg to archive the same functionality.
Is this user-facing behavior change?
Yes, we remove an existing feature. Due to recent bug reports I believe almost nobody was using it.
But as states, this also enables community pkgs to replicate the existing behavior.
Did you use Generative AI?
Yes, SWE-1.7