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

Include tracepoints by default on Linux #31

Merged

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Jan 26, 2023

Part of ros2/ros2#1177

The main change here is to explicitly depend on/expect LTTng-UST on Linux.

This also adds a new TRACETOOLS_TRACEPOINTS_EXCLUDED CMake option. It allows users to keep the instrumentation calls (e.g., rclcpp->tracetools) without including the actual tracepoints.

The GitHub workflows and README were also updated.

Note that the CMake warning about pytest-cov in the Rpr job (which makes it look like it failed here) will be fixed by ament/ament_cmake#436.

Signed-off-by: Christophe Bedard christophe.bedard@apex.ai

@christophebedard christophebedard self-assigned this Jan 26, 2023
@christophebedard
Copy link
Member Author

cc @iluetkeb @mjcarroll

@christophebedard
Copy link
Member Author

This is a draft because there are things we need to figure out.

Currently, there are 3 possible configurations:

  1. TRACETOOLS_DISABLED is set to ON: calls to tracetools (e.g., TRACEPOINT(...)) are preprocessed out, and tracetools doesn't exist
    • This is the current behaviour on Windows
  2. TRACETOOLS_DISABLED is set to OFF and LTTng-UST is not installed: TRACEPOINT() calls to tracetools are included, but they are empty (the actual tracepoints are compiled out)
    • Internally, TRACETOOLS_LTTNG_ENABLED is set to FALSE
    • This is the current behaviour when using the Linux binaries
  3. TRACETOOLS_DISABLED is set to OFF and LTTng-UST is installed: TRACEPOINT() calls to tracetools are included, and they call the actual tracepoints (LTTng's tracepoint())
    • Tracepoints still need to be enabled for data to be collected
    • Internally, TRACETOOLS_LTTNG_ENABLED is set to TRUE
    • This would be the new default behaviour on Linux

If we expect LTTng to be installed on Linux, then we don't really need (2) and can remove TRACETOOLS_LTTNG_ENABLED. However, there could still be value in keeping (2) by exposing an equivalent to TRACETOOLS_LTTNG_ENABLED as a CMake option. This would allow keeping empty calls to tracetools, which would act as a kind of no-op version of tracetools. The shared lib could then be swapped to one that triggers the tracepoints. This was always one of the intended "features" of tracetools, although in practice I've never heard of anyone leveraging this.

Therefore, I'd like to get some input on this.

@iluetkeb
Copy link
Contributor

The current change looks good to me -- it makes the most useful use case easier to use, while maintaining all the various disabling options. People have asked for these in the past, so I think keeping them around is a good choice.

@mjcarroll
Copy link
Member

Keeping the full range of configurations makes sense to me.

In the case that tracetools may eventually support other tracing systems (on platforms where lttng isn't supported), then we should have a package-level switch to enable/disable tracing that will set correct configurations for alternative tracers.

@clalancette
Copy link
Contributor

I guess we can keep all of them, though I think that someone using it in method 2 is pretty unlikely. By the time you've set that up, you may as well have just built from source. But I'm not opposed to keeping it.

@christophebedard
Copy link
Member Author

Seems like we're in favour (or not in disfavour) of keeping configuration (2) and exposing the corresponding CMake variable as a CMake option.

I've changed TRACETOOLS_LTTNG_ENABLED to TRACETOOLS_TRACEPOINTS_EXCLUDED to make it more generic (instead of LTTng-specific) and so that it is disabled/off by default. I used "excluded" instead of "disabled" to avoid potential confusion with runtime tracepoints disabling/enabling, even if the CMake option description makes it quite clear. See a79e873

In the case that tracetools may eventually support other tracing systems (on platforms where lttng isn't supported), then we should have a package-level switch to enable/disable tracing that will set correct configurations for alternative tracers.

So the tracer would then be automatically selected based on the platform? Would it be useful to be able to select between multiple tracers for a single platform, e.g., -DTRACETOOLS_TRACER=lttng vs -DTRACETOOLS_TRACER=sdt vs -DTRACETOOLS_TRACER= (for an equivalent to -DTRACETOOLS_TRACEPOINTS_EXCLUDED=ON), all on Linux? We're really far from actually needing something like this, but I thought I'd mention it.

@christophebedard christophebedard force-pushed the christophebedard/include-tracepoints-by-default branch from 5ef06e6 to 9730555 Compare January 31, 2023 19:14
@christophebedard christophebedard force-pushed the christophebedard/include-tracepoints-by-default branch 5 times, most recently from 474e877 to 20ef0c6 Compare March 24, 2023 00:35
@christophebedard christophebedard marked this pull request as ready for review March 24, 2023 00:40
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard christophebedard force-pushed the christophebedard/include-tracepoints-by-default branch from 20ef0c6 to e83df5c Compare March 29, 2023 16:24
* Remove matrix option for LTTng installed/uninstalled
* Add matrix option for TRACETOOLS_TRACEPOINTS_EXCLUDED
* Update sanity tests to check that (re)building with/without
  -DTRACETOOLS_TRACEPOINTS_EXCLUDED=ON works

Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
See ros-tooling/setup-ros#535

Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard christophebedard force-pushed the christophebedard/include-tracepoints-by-default branch from e83df5c to 8e335a9 Compare March 29, 2023 16:26
@christophebedard
Copy link
Member Author

christophebedard commented Mar 29, 2023

CI building everything using ros2/ci#690:

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

@christophebedard christophebedard merged commit 92da222 into rolling Apr 3, 2023
@christophebedard christophebedard deleted the christophebedard/include-tracepoints-by-default branch April 3, 2023 15:12
@christophebedard
Copy link
Member Author

Coincidentally, this was the 1000th ros2_tracing commit 😁

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.

4 participants