-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tracing instrumentation using tracetools #294
Add tracing instrumentation using tracetools #294
Conversation
cc49bef
to
c2d6771
Compare
Oh, looks like this repo aims to support Iron, Jazzy, and Rolling on the |
c2d6771
to
a1db0ea
Compare
@Yadunund @clalancette any interest in having this? |
@christophebedard we'd love to have this! We're sort of freezing the |
@christophebedard do you mind resolving the conflicts after rebasing to the latest |
a1db0ea
to
66cdb1f
Compare
I rebased, but now the tracing tests in |
66cdb1f
to
11a0ea5
Compare
I rebased & resolved the new merge conflicts. I also added client/service instrumentation, see ros2/ros2_tracing#145.
I ran the tests quite a few times and didn't get any failures. Not sure what changed in the code, other than the instrumentation. |
11a0ea5
to
9ddd3cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left 3 more minor things to fix, then I'm happy with it.
1587c67
to
1b65002
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more fix, then I'm happy with this.
Signed-off-by: Christophe Bedard <[email protected]>
With the recent changes to get data out of it, and the change to zenoh-cpp, it can mostly be replaced with a couple of inline pieces of code. However, we do add in a new function to help us explicitly get system clock time from std::chrono in nanoseconds. Signed-off-by: Chris Lalancette <[email protected]>
1b65002
to
4fa479f
Compare
Signed-off-by: Christophe Bedard <[email protected]>
4fa479f
to
ebc61f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks for all of the iteration, and for being patient with us as we revamped other things. I'm going to go ahead and merge now.
@christophebedard |
Make sure to build the |
Oh, I guess this does break Jazzy builds. This will only work with Rolling (built from source for now). The new service/client tracepoints would need to be excluded on Jazzy. |
So CI passed for this PR because the Since this repo doesn't use distro-specific branches yet, could we add distro-specific |
With this instrumentation,
rmw_zenoh_cpp
is supported out-of-the-box by tools that process ROS 2 trace data, such as Eclipse Trace Compass.It's pretty straightforward except for a few things:
rmw_publisher_t *
for thermw_publish
tracepoint andrmw_service_t *
for thermw_send_response
tracepointPublisherData
andServiceData
, respectively.rmw_publish
andrmw_send_response
tracepointsrmw
. This means we don't need to instrument the underlying middleware itself, which we used to have to do.create_map_and_set_sequence_num()
, since it's already pretty simple and it makes extracting the source timestamp easier.rmw
GID for thermw_subscription_init
tracepointI'm open to suggestions.
action-ros-ci-repos-override: https://gist.githubusercontent.com/christophebedard/18561011c03603652ad49701412e5f41/raw/9e9d0bc9bbf0c36b7d5419bc5797ef17aa90d6fa/ros2.repos