Add timestamp to rmw_publish tracepoint

[cwecht]

See #73

Corresponding rmw implementation PRs:

• Add timestamp to rmw_publish tracepoint rmw_cyclonedds#454
• Add timestamp to rmw_publish tracepoint rmw_fastrtps#694
• Add ros2_tracing tracepoints rmw_connextdds#120

action-ros-ci-repos-override: https://gist.githubusercontent.com/christophebedard/124d8311bed71f7820a2185ce0a25cc3/raw/49c81e6cacce1a5e621ed3880b205dac9096a3ac/ros2.repos

[christophebedard]

Sorry for the very late follow-up here. Good to see that the rmw_* & DDS implementation maintainers are in favour of this change.

You can remove rmw_publisher_init & rmw_subscription_init since they are not needed if we remove (the need for) the DDS tracepoints. See #73 (comment). A rebase would also be good. On second thought, I would leave them in (just like you did). Could be useful for people who use custom middleware instrumentation, since they allow linking publishers/subscriptions to their corresponding middleware object, so there is no real need to remove them.

My only concern is that this breaks API/ABI. Tracepoints are effectively part of the tracetools API and ABI because they are exposed as normal functions, since TRACETOOLS_TRACEPOINT() macros expand to normal function calls. I think we usually use the tick-tock strategy for this kind of breaking change. In the past, for the sake of API/ABI stability, we've kept unused tracepoint fields: see rclcpp_publish. However, given that this is a real improvement and that there is no good, "clean" way to deal with this breaking change, I would personally lean towards just going ahead with this in Rolling, but I want to check with other core maintainers. @clalancette what do you think?

[clalancette]

However, given that this is a real improvement and that there is no good, "clean" way to deal with this breaking change, I would personally lean towards just going ahead with this in Rolling, but I want to check with other core maintainers. @clalancette what do you think?

I have a few different thoughts about this:

1. It looks like the changes to rmw_connextdds haven't been approved yet. I'd want to make sure we have agreement from @asorbini before going forward there.
2. The biggest problem with changing the tracepoints like this would be affecting the out-of-core RMWs. However, from what I can tell the only out-of-core RMW that uses TRACEPOINT is https://github.com/christophebedard/rmw_email, so I guess we are OK there :).
3. We could consider adding a new tracepoint, rmw_publish_with_timestamp or whatever. Then we could switch the in-core RMWs to use that. But it is probably not worth it.

Overall, I'm OK with this change, as long as all of the changes to the in-core RMWs are ready and merged at the same time.

[christophebedard]

Sounds good!

2. The biggest problem with changing the tracepoints like this would be affecting the out-of-core RMWs. However, from what I can tell the only out-of-core RMW that uses TRACEPOINT is https://github.com/christophebedard/rmw_email, so I guess we are OK there :).

I'm sure the author of that RMW implementation wouldn't mind 😄

[christophebedard]

You can remove rmw_publisher_init & rmw_subscription_init since they are not needed if we remove (the need for) the DDS tracepoints.

If we weren't changing an rmw tracepoint's fields, we probably wouldn't want to remove these other rmw tracepoints, but we are, so removing them doesn't really make a huge difference. However, we could keep them in case we/people want to instrument middleware implementations for other purposes in the future. Then they could still be used to link ROS 2 pubs/subs to the corresponding middleware objects. So I might leave them in for now.

[cwecht]

1. It looks like the changes to `rmw_connextdds` haven't been approved yet.  I'd want to make sure we have agreement from @asorbini before going forward there.

Please note that rmw_connextdds hadn't introduced the ros2_tracing tracepoints before ros2/rmw_connextdds#120. From my point of view, rmw_connextdds shouldn't really block us here.

[christophebedard]

I agree, but it would block us if the proposed design is rejected/doesn't work for rmw_connextdds. However, looks like it should be good.

[cwecht]

I agree, but it would block us if the proposed design is rejected/doesn't work for rmw_connextdds. However, looks like it should be good.

ros2/rmw_connextdds#120 has now been approved, too. So every "major RMW implementation" has approved the proposed changes now. However, in the meantime this arised: ros2/rmw_cyclonedds#454 (comment) . IMO we should be fine for the time being with the proposed solution, but I'd like to have @christophebedard s approval here.

[mjcarroll]

@christophebedard can we get one more 👍 here

[cwecht]

I agree, but it would block us if the proposed design is rejected/doesn't work for rmw_connextdds. However, looks like it should be good.

ros2/rmw_connextdds#120 has now been approved, too. So every "major RMW implementation" has approved the proposed changes now. However, in the meantime this arised: ros2/rmw_cyclonedds#454 (comment) . IMO we should be fine for the time being with the proposed solution, but I'd like to have @christophebedard s approval here.

Quick update on this one: it turned out, that the issue mentioned in ros2/rmw_cyclonedds#454 (comment) could be fixed by using a slightly different API from cyclone: ros2/rmw_cyclonedds#454 (comment) . At this point its probably more on the rmw_cyclonedds maintainers to give their approval regarding this change.

[christophebedard]

Looks good from my end! Just waiting on an approval for the last tiny change for the Cyclone DDS PR and then I'll probably run CI again. However, if you could amend and force-push here (without changing anything), it would correctly run local CI with the corresponding rmw PRs.

test_tracetools' test_publisher.py and test_pub_sub.py should be updated to cover the new rmw_publish fields, but that could be done in another PR (I could even do it), I don't want to delay this PR any more 😅

[cwecht]

We've got the CycloneDDS PR approved: ros2/rmw_cyclonedds#454 (review)

[christophebedard]

Full CI:

• Linux
• Linux-aarch64
• Windows

[christophebedard]

ROS 2 CI looks good. I don't have the necessary permissions to merge the rmw PRs, so I'll let @mjcarroll (or anyone else who can) merge everything at once.

[christophebedard]

@mjcarroll friendly ping

[christophebedard]

Thanks for merging! I'll create a new release now.

Thanks for the contribution @cwecht!

[christophebedard]

I'll create a new release now.

ros/rosdistro#39635

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-jazzy-jalisco-released/37862/9