Are there any special flags which should be passed to cmake for the tracingtools? I've seen on the gitlab page some instructions of -DWITH_LTTNG=ON.

That flag was for the ROS 1 version of tracetools. In this version, tracing is automatically enabled if LTTng is found by CMake. In the future it will be enabled automatically if LTTng is found. We have a patch for that, but it's not merged yet.

Anyway, what this means is that the tracepoints in rcl and rclcpp can call tracetools, but nothing happens. Then, somebody who wants to trace just installs LTTng, and then only has to recompile tracetools in the local workspace (as opposed to having to recompile everything).

In the future, we would like to discuss whether tracing could always be enabled (tracing is purposefully very little overhead), but that's a matter for latter.

Regarding breaking CI: We can use a tag or a branch instead of master. If we use a tag, that means a lot of PR's for you guys, but it's totally safe, since you know it works. If we use a branch, it's a bit more risk but not much. Thus, I would prefer using a branch. Okay?

Regarding control: What would you need to feel like you're in control? Should we put this into rcl directly (it's not big, and all dependencies are optional)? Should we have the repo in the ros2 org?

Regarding performance: None of the tracepoints are in critical places that are called very often. No inner loops or something the like. In principle, if you have a timer or the like that's extremely short, the tracepoint could add a measureable (though still not significant) overhead. In general, though, performance is a subject we could certainly look at more, if you're worried about it.

If we use a tag, that means a lot of PR's for you guys, but it's totally safe, since you know it works. If we use a branch, it's a bit more risk but not much. Thus, I would prefer using a branch. Okay?

The nice thing about a pr for each time a new version comes up is that it's an opportunity for us to run CI on the new state and compare it to the old. The goal is really to minimize downtime and "known flaky or failing tests" on our "master" so that other on going work is not disrupted.

I would personally prefer using tagged versions, especially since I don't expect this dependency to be changing frequently (at least not over the long term), but that's just my opinion. Maybe others on the team would feel differently. In contrast, we put up with the downsides of using master from Fast-RTPS because it is still changing so much and we often need intermediate fixes (fixes between releases).

Alternatively, if every change to your repository had to pass our CI before being merged to master, that would be ok too. But that puts a continuous burden on you guys and taxes our CI resources as well.

Regarding control: What would you need to feel like you're in control? Should we put this into rcl directly (it's not big, and all dependencies are optional)? Should we have the repo in the ros2 org?

This is really a non-issue for me. I'm totally fine for you guys to own it and host it. The point is that we may choose to fork it or pin the version to an older release when doing our own release if we run into issues. Whereas if it were one of our packages we would likely fix the problem and author a new release right then and there. But again this is only a problem if there is an issue and you guys cannot respond to the problems quickly, neither of which I anticipate over the long run.

My opinion is that nothing needs to be done here.

Regarding performance: None of the tracepoints are in critical places that are called very often. No inner loops or something the like. In principle, if you have a timer or the like that's extremely short, the tracepoint could add a measureable (though still not significant) overhead. In general, though, performance is a subject we could certainly look at more, if you're worried about it.

I definitely don't want to do premature optimization, and I think having the call out to your library is fine in most cases, even like our official Debian packages, but at the same time I think it should be possible for someone building ROS 2 from source to completely compile out the trace lines from rcl and rclcpp (and even avoid needing to build or link your library) if they decide they want to do so.

Alright, we'll tag it and update this PR.

I definitely don't want to do premature optimization, and I think having the call out to your library is fine in most cases, even like our official Debian packages, but at the same time I think it should be possible for someone building ROS 2 from source to completely compile out the trace lines from rcl and rclcpp (and even avoid needing to build or link your library) if they decide they want to do so.

Disabling this completely would be relatively trivial, since "TRACEPOINT" is already a macro, so we could add a compile time setting to the tracetools and define it as empty.

Yeah, making it completely optional is probably not necessary (at least until someone asks for it), but I do want to make sure that people compiling from source can completely compile it out, which it sounds like we could do in the header of ros2_tracing.

Alright, we'll tag it and update this PR.

Is this still happening? If so I'll wait to add my ✔️ until then. And it would make sense to test the new package passes CI.

Alright, we'll tag it and update this PR.

Is this still happening?

Oh yeah, that would be good to do first. Obviously passing CI too.

Not sure if we need to merge this with the above mentioned pr's at the same time or not, but here's CI just up to the tracetools*/ros2trace packages to see how it looks so far:

• Linux
• Linux-aarch64
• macOS
• Windows

The failure on Windows should be fixed by ros2_tracing!71 (decoding issue with "ü"/0xfc in setup.py files). Updated to 0.2.1 😄

btw... I think you may have a problem on your Windows build farm. The "setup.py" is utf-8 and contained an utf-8 special character, which works fine on Linux, but not on Windows. Interestingly, this only breaks when reading from stdin. Usually this means that some terminal does have a utf-8 encoding when you're expecting it to be.

Christophe found an interesting stackoverflow entry related to terminal encoding, which also specifies how you can use the terminal's encoding on stdin.

I think you may have a problem on your Windows build farm.

I don't think the problem is on the buildfarm. It is reasonable to expect the build to pass independently what locale a machine has set. The logic extracting the metadata in from the setup.py file should work independent of the default encoding. colcon/colcon-python-setup-py#19 tries to achieve that:

• Windows
  • While the build still fails it resolves the encoding problem.
  • The remaining failure might need to be addressed in tracetools.

Therefore I would suggest to revert the changes to replace the non-ASCII characters in your repo.

The remaining failure might need to be addressed in tracetools.

Looks like it. However, I'm a bit puzzled: The library is correctly created as a dynamic library ("tracetools.dll") but then the build for the status tool tries to read "tracetools.lib", which would be the name if the library were static.

Edit: This is apparently normal. The lib file is also generated for dynamic libraries, except we had not yet added visibility markers, so nothing was exported. In that case, the .lib file is not generated.

Fixed now, we'll make a new tag soon.

Yeah, as you said, usually this is due to lacking dll export/import attributes on your classes and functions.

New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

The unicode decode error is back...

We reverted our change on @dirk-thomas 's request, but apparently the fix in colcon isn't working?

I think it's not using the right .repos file. It's testing b9ff095/0.2.0 (separate from the encoding issue).

I think it's not using the right .repos file. It's testing b9ff095/0.2.0 (separate from the encoding issue).

Where did you see that @christophebedard ?

I think it's not using the right .repos file. It's testing b9ff095/0.2.0 (separate from the encoding issue).

Where did you see that @christophebedard ?

The job's build information:

repos_url: https://raw.githubusercontent.com/ros2/ros2/b9ff095e2bef4cadfa5cec03b7ef49d56c530e7d/ros2.repos

I think it's not using the right .repos file.

Correct, the passed hash is still pointing to the 0.2.0 release. I will trigger another build using the latest: https://raw.githubusercontent.com/ros2/ros2/f05f0d2183608d491d9fa709dd35589871e13657/ros2.repos

but apparently the fix in colcon isn't working?

The fix has only been merged into master but not yet released in a new package version. Therefore the job needs to be triggered with explicitly using master branches from colcon (rather than the released packages):

• Windows

Sorry, that's my bad, copied the repo's file from the old test without thinking about it.

Looks like the Windows job has some compiler warnings still, but otherwise it's looking ok.

Looks like the Windows job has some compiler warnings still, but otherwise it's looking ok.

Fixed with 0.2.3!

@christophebedard Friendly ping. Is there an updated version we should test again?

@dirk-thomas sure, could you test this one pointing to a specific commit with a fix? https://gitlab.com/micro-ROS/ros_tracing/ros2/raw/a4a32c7b653014a69686713a4b191550669f25b4/ros2.repos

I think @iluetkeb just got back from vacation, so we may be able to create a new release sometime soon.

could you test this one pointing to a specific commit with a fix?

• Linux (known flaky test)
• Linux-aarch64
• macOS
• Windows (same test failures are present on master)

So, this is looking good, hmm?

I've reviewed Christophe's merge requests, all okay there as well.

Updated to 0.2.7.

Updated to 0.2.7.

• Linux
• Linux-aarch64
• macOS
• Windows

At first glance the Windows test failure do not look related to our work. Or am I missing something?

At first glance the Windows test failure do not look related to our work. Or am I missing something?

@cottsay I will let Scott comment if all those test failures are expected.

I will let Scott comment if all those test failures are expected.

Looks good to me.

@wjwwood @nuclearsandwich @cottsay @dirk-thomas I've updated the version to 0.2.8 following the changes made for ros2/rclcpp#789 and I also squashed everything.

I've updated the version to 0.2.8

Here's new CI:

• Linux
• Linux-aarch64
• macOS
• Windows

I think this pr needs to be rebased, since we had to update the repos file already to pin the version of fastrtps...

I think this pr needs to be rebases, since we had to update the repos file already to pin the version of fastrtps...

Done!

Ok, CI again:

• Linux
• Linux-aarch64
• macOS
• Windows

Seems like test failures are not related to this 😃

@cottsay can you check the latest CI and confirm those are expected failures?

@cottsay can you check the latest CI and confirm those are expected failures?

I don't know if I'd use the term "expected", but we're seeing all of those in nightlies 😛

Ok, I guess this is good to merge then.

I think ros2/rcl#473 and ros2/rclcpp#789 are not needed to merge this pr, if anyone thinks that's not right, let me know.