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

Switch to custom lttng-ctl Python bindings #81

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Oct 26, 2023

Requires ros2/ci#725 for CI

This replaces the lttng-ctl Python bindings (python3-lttng), on which the ros2 trace command and Trace launch action rely, with a new implementation.

The new lttng-ctl Python bindings implementation, lttngpy, uses pybind11 to bind C++ code to Python. The bindings do not cover the whole trace control library (liblttng-ctl, from the liblttng-ctl-dev package on Ubuntu), but they cover whatever is needed by tracetools_trace/ros2trace. A lot of lttngpy is just exposing C functions and constants from lttng-ctl (which usually start with lttng_* or LTTNG_*) to Python. However, lttngpy does wrap some lttng-ctl functions to provide a more Pythonic interface, e.g., Optional[...] arguments and Union[...] return types.

I replaced the calls to the current Python bindings (lttng) in tracetools_trace (and some other locations) with calls to lttngpy. It is (almost) a simple drop-in replacement; users should not notice any difference. In fact, this even improves the usefulness of ros2 trace/Trace, since they now support enabling perf counters, e.g., ros2 trace -c perf:thread:task-clock. Indeed, the bindings available from python3-lttng do not completely cover liblttng-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 from apt 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.

@christophebedard christophebedard self-assigned this Oct 26, 2023
@christophebedard christophebedard force-pushed the christophebedard/write-lttng-python-bindings branch from f6c842e to ca83325 Compare October 26, 2023 23:27
@christophebedard
Copy link
Member Author

@iluetkeb as we discussed

Copy link
Contributor

@iluetkeb iluetkeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, thanks :-)

@christophebedard christophebedard added the enhancement New feature or request label Oct 27, 2023
Copy link
Member

@mjcarroll mjcarroll left a 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

lttngpy/src/lttngpy/channel.cpp Show resolved Hide resolved
@christophebedard
Copy link
Member Author

christophebedard commented Nov 7, 2023

Testing everything above lttngpy with ros2/ci#725:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@christophebedard
Copy link
Member Author

Oh, right, I need to install liblttng-ctl-dev in CI.

@christophebedard
Copy link
Member Author

@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.

@mjcarroll
Copy link
Member

https://github.com/ros2/ci/tree/christophebedard/install-liblttng-ctl-dev

@christophebedard
Copy link
Member Author

RHEL test job:

  • Linux RHEL Build Status

@christophebedard
Copy link
Member Author

The RHEL job fails due to another issue, see ros2/ci#725 (comment).

@christophebedard christophebedard force-pushed the christophebedard/write-lttng-python-bindings branch from ca83325 to 6ebda24 Compare November 22, 2023 18:33
Signed-off-by: Christophe Bedard <[email protected]>
Signed-off-by: Christophe Bedard <[email protected]>
@christophebedard christophebedard force-pushed the christophebedard/write-lttng-python-bindings branch from 6ebda24 to 49b6dba Compare December 14, 2023 18:17
Signed-off-by: Christophe Bedard <[email protected]>
@christophebedard christophebedard force-pushed the christophebedard/write-lttng-python-bindings branch from 4a870d0 to b882f82 Compare December 14, 2023 21:13
@christophebedard
Copy link
Member Author

Let's try again:

Testing everything above lttngpy with ros2/ci#725:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Linux RHEL Build Status

@christophebedard
Copy link
Member Author

Looking good now, RHEL included.

@mjcarroll could you review the latest changes, i.e., the last commit?

@clalancette clalancette merged commit ed6f435 into rolling Jan 10, 2024
8 of 9 checks passed
@clalancette clalancette deleted the christophebedard/write-lttng-python-bindings branch January 10, 2024 18:41
@ros-discourse
Copy link

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

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

Successfully merging this pull request may close these issues.

5 participants