Skip to content
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 publishing instrumentation using tracetools #324

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Jun 28, 2021

Requires https://gitlab.com/ros-tracing/ros2_tracing/-/merge_requests/249

This adds instrumentation for the most common rmw_publish* function.

This goes along instrumentation for rclcpp (ros2/rclcpp#1600) and rcl (ros2/rcl#905). The goal is to track messages from the user code down to DDS and the network.

action-ros-ci-repos-override: https://gist.githubusercontent.com/christophebedard/e8b4c9567a7261ed955c67d7de6cd1ff/raw/d7deda7d7294b21d27a08dfbdcb19fdc1192cb76/ros2.repos
action-ros-ci-repos-override: https://raw.githubusercontent.com/ros2/rmw_cyclonedds/master/.github/resources/local.repos

Signed-off-by: Christophe Bedard bedard.christophe@gmail.com

@christophebedard
Copy link
Member Author

Once this is approved and once CI looks good, we will merge the ros2_tracing PR and create a new release.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Member Author

On second thought, I'm going to hold off on this PR and wait until I have a more complete instrumentation set (I will probably need to add more).

I would however of course still like to know if there is any objection to this.

I'm also working on instrumenting Cyclone DDS, although I'm not necessarily expecting to have the tracing instrumentation included in Cyclone DDS directly like it is for the core of ROS 2 (unless this is something that the Cyclone DDS maintainers want). cc @eboasson

@christophebedard christophebedard marked this pull request as draft June 30, 2021 16:52
@eboasson
Copy link
Collaborator

eboasson commented Jul 1, 2021

Hi @christophebedard

I would however of course still like to know if there is any objection to this.

I haven't played with the tracing tools used and so I can't comment on the value of a tracepoint like this, but as a general principle I am in favour of observability and having well-defined tracepoints in strategic places is therefore something I favour.

I'm also working on instrumenting Cyclone DDS, although I'm not necessarily expecting to have the tracing instrumentation included in Cyclone DDS directly like it is for the core of ROS 2 (unless this is something that the Cyclone DDS maintainers want). cc @eboasson

What I have always wanted to do, but never got around to ... is to define static trace points that work with at least dtrace and bpftrace as that would allow tracing all the way down to the network interfaces on the platforms I care most about. In my understanding, that means defining USDTs. A quick search suggests those can also be used with LTTng, which I think the ROS 2 tracing tools are built upon.

That suggests it is possible to instrument Cyclone DDS in a manner that makes sense for Cyclone DDS while also integrating nicely with ROS 2 instrumentation. If that can be done in a clean way, your work on instrumenting Cyclone is definitely something I am interested in.

@christophebedard
Copy link
Member Author

I haven't played with the tracing tools used and so I can't comment on the value of a tracepoint like this, but as a general principle I am in favour of observability and having well-defined tracepoints in strategic places is therefore something I favour.

Great!

What I have always wanted to do, but never got around to ... is to define static trace points that work with at least dtrace and bpftrace as that would allow tracing all the way down to the network interfaces on the platforms I care most about. In my understanding, that means defining USDTs. A quick search suggests those can also be used with LTTng, which I think the ROS 2 tracing tools are built upon.

That suggests it is possible to instrument Cyclone DDS in a manner that makes sense for Cyclone DDS while also integrating nicely with ROS 2 instrumentation. If that can be done in a clean way, your work on instrumenting Cyclone is definitely something I am interested in.

Yeah that's a great idea! I'll look into that then, thank you!

@christophebedard
Copy link
Member Author

So I looked into it, and the support for SDT probes in LTTng is only partial: it doesn't support probe parameters and the probes must not be guarded by semaphores. However, it turns out that LTTng can quite easily (although it requires building it from source) generate SDT probes alongside LTTng tracepoints, so I'm going to use that.

As for this PR, I'm just going to close it and replace it with a bigger PR with a more complete instrumentation set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants