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

Basic trace instrumentation #789

Merged
merged 1 commit into from Oct 18, 2019

Conversation

@iluetkeb
Copy link
Contributor

commented Jul 19, 2019

This is the first part of the instrumentation for the work described at https://discourse.ros.org/t/alpha-release-of-ros-2-tracing-tools/9833

@iluetkeb iluetkeb changed the title Master Basic trace instrumentation Jul 19, 2019
@christophebedard

This comment has been minimized.

Copy link

commented Jul 19, 2019

Also see ros2/rcl#473

@nuclearsandwich nuclearsandwich requested review from hidmic and ivanpauno and removed request for hidmic and ivanpauno Aug 15, 2019
@nuclearsandwich

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Apologies for the churn. Keyboard and mouse hand were not cooperating

Copy link
Contributor

left a comment

@iluetkeb one extra question. I've seen these tracepoints declared back in the tracetools package and it makes me wonder about the scalability of these tools. Is that a limitation of the current implementation or is it actually impossible to push those declarations closer to the relevant packages?

rclcpp/include/rclcpp/subscription.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/any_subscription_callback.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved
@hidmic

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@iluetkeb friendly ping.

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@iluetkeb one extra question. I've seen these tracepoints declared back in the tracetools package and it makes me wonder about the scalability of these tools.

What do you mean by "scalability"?

Is that a limitation of the current implementation or is it actually impossible to push those declarations closer to the relevant packages?

With "relevant packages", you mean rcl and rclcpp? Sure, we could put them in there. However, that would also meant that we would have to put the supporting code (which currently uses LTTng) in those packages as well. It's not big, but we figured by having one more indirection, it would overall be easier. Also easier to change and support other tracing packages in the future.

Moreover, right now, tracing is a noop by default. You have to recompile tracetools, but only tracetools, to change that. If we put it into rcl/rclcpp, we would have to recompile those, which is much more of an effort.

Update: One more note on the last item: Ordinarily, tracing calls are meant to be "always in there", but are only active (and thus consuming resources) when they are explicitly enabled at runtime. However, we have been requested (and Tully even reinforced this) to implement this in a way that it can be completely compiled out. I, personally, don't think that is really necessary, and that we could always have it in there, since the trace locations are not time-critical. And then we could just rely on the regular tracing tools to enable it, instead of having to recompile. However, that's not the way it's currently done, so at least recompiling isn't hard, when it's just tracetools.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

What do you mean by "scalability"?

To be forced to commit a each new tracepoint to the tracetools package doesn't lend itself to reuse by interested individuals wanting to trace their packages. I'm assuming here you intend to open this up for the community to use. In any case, it has little to do with this PR.

With "relevant packages", you mean rcl and rclcpp? Sure, we could put them in there. However, that would also meant that we would have to put the supporting code (which currently uses LTTng) in those packages as well. It's not big, but we figured by having one more indirection, it would overall be easier. Also easier to change and support other tracing packages in the future.

One more note on the last item: Ordinarily, tracing calls are meant to be "always in there", but are only active (and thus consuming resources) when they are explicitly enabled at runtime. However, we have been requested (and Tully even reinforced this) to implement this in a way that it can be completely compiled out.

I understand. I guess what I'm asking is if it's possible for someone to add a new tracepoint outside tracetools, regardless of how we do it for rcl and rclcpp today.

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

To be forced to commit a each new tracepoint to the tracetools package doesn't lend itself to reuse by interested individuals wanting to trace their packages.

Ah! You're thinking of people adding new tracepoints, right?

In the tracing framework we're currently using, LTTng, anybody can define tracepoints wherever and however they like, and LTTng will take care of aggregating the data. So, people can add custom tracepoints to their apps directly using LTTNg without having to touch tracetools.

That said, since ROS 2 is cross-platform, and LTTng is Linux only, the tracetools would be a possible place to implement a cross-platform tracing layer. For example, I haven't used it myself, but I know that Event Tracing for Windows is, in theory, supposed to also be able to emit user-space events. We haven't thought about this much, but if we want to go that way, we might improve tracetools a bit to make that sort of thing easier.

In the past, i.e. for ROS 1, we had a generic "message received" tracepoint. It wasn't very capable, but at least you had something similar to logging integrated with the rest of the tracing. Maybe we should bring that back as a first step.

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@hidmic @dirk-thomas @nuclearsandwich is this being considered for E?

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

@iluetkeb I was expecting you guys to resolve pending review comments.

About getting this into Eloquent, both API and feature freeze are happening on Oct 14th Oct 18th. You can check the distro roadmap here.

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@iluetkeb I was expecting you guys to resolve pending review comments.

Ah, I thought none were open, but I see that in one case, we wanted to rename a function. Is that it?

Sorry, but it would really help us, if you could state after Christophe's comments whether that is an OK solution for you or not.

About getting this into Eloquent, both API and feature freeze are happening on Oct 14th. You can check the distro roadmap here.

Thanks for the link. I see Oct 18th for feature freeze there. We could probably make the 14th as well, but could you say which is correct, please?

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

there also seems to be a problem with the dependency statement

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

if you could state after Christophe's comments whether that is an OK solution for you or not.

Done.

I see Oct 18th for feature freeze there. We could probably make the 14th as well, but could you say which is correct, please?

Doh, my bad. Oct 18th is the correct date.

@iluetkeb iluetkeb closed this Oct 11, 2019
@iluetkeb iluetkeb force-pushed the boschresearch:master branch from 5cc95fa to 07cb443 Oct 11, 2019
@iluetkeb iluetkeb reopened this Oct 11, 2019
@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

@christophebedard @hidmic I don't understand where the error resolving the "tracetools" rosdep key comes from. We have "tracetools" released both as a package and included in ros2/ros2#748.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

I don't understand where the error resolving the "tracetools" rosdep key comes from. We have "tracetools" released both as a package and included in ros2/ros2#748.

The failing job is an Eloquent PR job. It builds the code for ROS Eloquent. As far as I can see you have not released tracetools into Eloquent yet.

@christophebedard

This comment has been minimized.

Copy link

commented Oct 11, 2019

Makes sense. I'll create a new version/release once we've addressed the comments on all PRs.

@iluetkeb iluetkeb closed this Oct 14, 2019
@iluetkeb iluetkeb force-pushed the boschresearch:master branch from 1456344 to 07cb443 Oct 14, 2019
@iluetkeb iluetkeb reopened this Oct 14, 2019
@iluetkeb iluetkeb closed this Oct 14, 2019
@iluetkeb iluetkeb force-pushed the boschresearch:master branch from 65e1f2e to 07cb443 Oct 14, 2019
@iluetkeb iluetkeb reopened this Oct 14, 2019
@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Sorry for the churn, too many remotes in one repo makes for the odd mistake.

@iluetkeb iluetkeb force-pushed the boschresearch:master branch from 6e4bbc1 to 6ed5ceb Oct 15, 2019
@christophebedard

This comment has been minimized.

Copy link

commented Oct 15, 2019

The failing job is an Eloquent PR job. It builds the code for ROS Eloquent. As far as I can see you have not released tracetools into Eloquent yet.

The PR for the release into Eloquent is up! ros/rosdistro#22679

* Add initial instrumentation
* Rename function registration method and elaborate on object copy issue
* Rely on get_symbol overload instead of using enable_if

Signed-off-by: Christophe Bedard <fixed-term.christophe.bourquebedard@de.bosch.com>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
Signed-off-by: Ingo Luetkebohle <ingo.luetkebohle@de.bosch.com>
@iluetkeb iluetkeb force-pushed the boschresearch:master branch from 6ed5ceb to 3ef5fc3 Oct 16, 2019
@hidmic
hidmic approved these changes Oct 16, 2019
Copy link
Contributor

left a comment

LGTM!

@wjwwood

This comment has been minimized.

Copy link
Member

commented Oct 16, 2019

I think this is also unblocked now that ros2/ros2#748 is merged.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2019

CI for rcl and rclcpp:

  • Linux Build Status
  • Linux aarch64 Build Status
  • MacOS Build Status
  • Windows Build Status
@christophebedard

This comment has been minimized.

Copy link

commented Oct 17, 2019

It's failing on Windows because rclcpp cannot resolve _demangle_symbol() and _get_symbol_funcptr(), which are defined in tracetools here and implemented here.

I suspect that it might be because they're not using TRACETOOLS_PUBLIC.

I made the fix, but can't test it on Windows. @hidmic or @wjwwood, if you think this fix looks good, could you run the CI again using 6fe7786e123b61c6c9934a73be9cca6914276778 for the ros2_tracing repo?

This is another situation where being able to completely preprocess out all tracing-related/instrumentation calls would be nice, but now is not really the time 😆

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

This is another situation where being able to completely preprocess out all tracing-related/instrumentation calls would be nice, but now is not really the time

I implemented that, but you have to specify the right option. Maybe we should set that option on Windows by default, until someone implements tracing on Windows.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

Re-running Windows CI against micro-ROS/ros2_tracing at 6fe7786e123b61c6c9934a73be9cca6914276778 febd57834ea9c9cb06324a5a18c3495c99fd61c7:

  • Windows Build Status
@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

@hidmic hold it, I've made more fixes

@iluetkeb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

Try febd57834ea9c9cb06324a5a18c3495c99fd61c7 instead

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

@iluetkeb if CI passes, you guys still have to update the ros2.repos on ros2/ros2 after merging (and tagging?) those fixes before we merge this one.

@christophebedard

This comment has been minimized.

Copy link

commented Oct 18, 2019

Windows CI passed!

I've created 0.2.9.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

One last CI run for rcl and rclcpp:

  • Linux Build Status
  • Linux aarch64 Build Status
  • MacOS Build Status
  • Windows Build Status
@hidmic

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

Alright, going in!

@hidmic hidmic merged commit 8fd9a0a into ros2:master Oct 18, 2019
1 of 2 checks passed
1 of 2 checks passed
Epr__rclcpp__ubuntu_bionic_amd64 Build finished.
Details
DCO DCO
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.