-
Notifications
You must be signed in to change notification settings - Fork 43
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
Switch to custom lttng-ctl Python bindings #81
Switch to custom lttng-ctl Python bindings #81
Conversation
f6c842e
to
ca83325
Compare
@iluetkeb as we discussed |
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.
Awesome work, thanks :-)
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.
Overall the approach looks good. While it isn't zero-cost, I think writing your own bindings is the right call in this situation, especially when dealing with LTS durations.
I had one small question of something that jumped out at me as kind of odd, but overall LGTM
Testing everything above |
Oh, right, I need to install |
@mjcarroll I don't have access to ros2/ci. Could you push my PR branch to the repo? Then I can re-run the CI jobs above using that CI branch, or you can do it if you want. |
The RHEL job fails due to another issue, see ros2/ci#725 (comment). |
ca83325
to
6ebda24
Compare
Signed-off-by: Christophe Bedard <[email protected]>
Signed-off-by: Christophe Bedard <[email protected]>
6ebda24
to
49b6dba
Compare
Signed-off-by: Christophe Bedard <[email protected]>
4a870d0
to
b882f82
Compare
Let's try again: Testing everything above |
Looking good now, RHEL included. @mjcarroll could you review the latest changes, i.e., the last commit? |
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 |
Requires ros2/ci#725 for CI
This replaces the lttng-ctl Python bindings (
python3-lttng
), on which theros2 trace
command andTrace
launch action rely, with a new implementation.The new lttng-ctl Python bindings implementation,
lttngpy
, usespybind11
to bind C++ code to Python. The bindings do not cover the whole trace control library (liblttng-ctl
, from theliblttng-ctl-dev
package on Ubuntu), but they cover whatever is needed bytracetools_trace
/ros2trace
. A lot oflttngpy
is just exposing C functions and constants fromlttng-ctl
(which usually start withlttng_*
orLTTNG_*
) to Python. However,lttngpy
does wrap somelttng-ctl
functions to provide a more Pythonic interface, e.g.,Optional[...]
arguments andUnion[...]
return types.I replaced the calls to the current Python bindings (
lttng
) intracetools_trace
(and some other locations) with calls tolttngpy
. It is (almost) a simple drop-in replacement; users should not notice any difference. In fact, this even improves the usefulness ofros2 trace
/Trace
, since they now support enabling perf counters, e.g.,ros2 trace -c perf:thread:task-clock
. Indeed, the bindings available frompython3-lttng
do not completely coverliblttng-ctl
. They are always lagging behind; I've fixed issues and contributed improvements before, but, since any new improvements are only going to be available fromapt
in the next Ubuntu LTS release, we cannot rely on this. Therefore, I think implementing our own bindings is a better solution.Note that RHEL has
liblttng-ctl
version 2.12.11, while Ubuntu has 2.13.x, so I had to adapt some things to support both 2.12 and 2.13.Note: The Sanity checks / binary (rolling) CI job will fail because
ros-rolling-lttngpy
is of course not yet available.