Implement service recording and display info about recorded services

[Barry-Xu-2018]

Split #1414 to 2 PRs.
This is first PR. This PR implement service record and recorded service info display.

⚠️ This PR depended on ros2/rclcpp#2209

• Relates service introspection event does not have unique GID during transaction rmw#357

[Barry-Xu-2018]

The comments are from MichaelOrlov (#1414 (review))
Comments related to this PR (marked as ✅) will be handled in this PR.

---

I have made a first pass of the review and here is my major notes and proposals:

1. ✅
   It seems we don’t have tests for the rosbag2_cpp::Info::read_service_info(...) and Python wrapper for information about services. It would be nice to add at least one sanity check.

2. ✅
   I have a proposal about new parameters for topic/services exclusions:

   Keep the old parameter exclude but rename it to the exclude_regex. it will be applicable to both topics and services.
   Make exclude_topics and exclude_services as std::vectorstd::string i.e. space separated list of topics and services names for CLI arguments.
   IMO it will be more user-friendly. We have had recently an incoming issue from one of the users when it was expected that the exclude parameter is a list of topics. Like opposite to the topics parameter.
   It looks intuitively reasonable to expect it to be defined this way.

3. Handle in Support service 2/2 --- rosbag2 service play #1481
   I would like to request to rename PlayerClient to the PlayerServiceClient and consider moving it to its own separate cpp and hpp files.
   The PlayerClient is pretty large and independent from its parent PlayerImpl class. We already have a lot of code inside the Player class and with this inner class inside it will become more cluttered and more difficult to navigate and support.

4. Handle in Support service 2/2 --- rosbag2 service play #1481
   We are doing deserialization for the same service message twice. Once in the PlayerClient::is_include_request_message(..) and the second time in the PlayerClient::async_send_request(). This is an expensive procedure.
   I would like to request structural changes there.
   Could you please rework is_include_request_message(..) to the get_service_request_message(serialized_message) and return smart pointer to the deserialized message? If there are no valid request message it will return a pointer to null. Then rework async_send_request(deserialized_message) to accept those smart pointer to the already deserialized message.

5. Handle in Support service 2/2 --- rosbag2 service play #1481
   I also have a concern about PlayerClient::async_send_request. We can’t really make it sending request asynchronously at least with the current design. Because we need to wait for future returning from client_->async_send_request(..). Since we can't delete ros_message until the future complete. Otherwise, it will be a dangling pointer to the request_addr and memory could be reused and we have a chance to use corrupted memory during the client_->async_send_request(..). Also according to the notes in the client_->async_send_request(..) API need to call client_->remove_pending_request(future) if we wait for future and timeout happened.
   As a first iteration, I would suggest to making sending client request synchronous with timeout. Then need to figure out how to design it to be asynchronous.
   I have idea to create some static worker thread and wait in it for static condition variable by timeout and check for future. If another request needs to be sent we can signal to condition variable to interrupt the wait earlier than timeout, remove_pending_request(future) and then try to send the new service request.

[Barry-Xu-2018]

@MichaelOrlov

• Item 1 in Implement service recording and display info about recorded services #1480 (comment)
  • Commit ffbfab3
• Item 2 in Implement service recording and display info about recorded services #1480 (comment)
  • Commit 62bd900

By now, all your comments have been handled.
Please review again.

[Barry-Xu-2018]

@MichaelOrlov

One case would like to discuss it with you.
While recording, user set --topics or --services and --regex at the same time.
Current process as below:

1. Check if topic or service is in topic list or service list.
2. Check if topic or service matches regular expression.

That is, the filtered topic or service must match 1 and 2 at the same time.
Whether is it better for user to change to match 1 or 2 ?

exclude parameter also has the same problem.

I'd like to hear your opinions.

[MichaelOrlov]

@Barry-Xu-2018 As regards

One case would like to discuss it with you.
While recording, user set --topics or --services and --regex at the same time.
Current process as below:

Check if topic or service is in topic list or service list.
Check if topic or service matches regular expression.
That is, the filtered topic or service must match 1 and 2 at the same time.
Whether is it better for user to change to match 1 or 2 ?

exclude parameter also has the same problem.

I'd like to hear your opinions.

I would expect if the user explicitly specifies --topics or --services and --regex at the same time then 1 or 2 logic should be applied as you mentioned above. Otherwise, it might be tedious for users to write a regex string which will satisfy both conditions.
Feel free to change the current logic for filtering in case if both topics or services and regex parameters are applied.

[MichaelOrlov]

@Barry-Xu-2018 Thank you for addressing the review comments.
I've run one more round of review. I found a few minor things to change.
Also, I have a concern about the way the tests are written in the test_info.cpp.
On one hand, using the generated bag files in tests is a working approach, however on the other hand it will be difficult to support such test infrastructure in the future.
Sometimes we make changes in the bag format of metadata and have to regenerate all bags written for tests.
It would be better to write bags on the fly from each test to the temporary folder.
Could you please change tests this way?
You can find an example of how to write test bags on the fly in the other tests in the rosbag2_cpp package. For instance this one.

TEST_P(ParametrizedTemporaryDirectoryFixture, info_for_standalone_bagfile) {
  const auto storage_id = GetParam();
  const auto bag_path = rcpputils::fs::path(temporary_dir_path_) / "bag";
  {
    // Create an empty bag with default storage
    rosbag2_cpp::Writer writer;
    rosbag2_storage::StorageOptions storage_options;
    storage_options.storage_id = storage_id;
    storage_options.uri = bag_path.string();
    writer.open(storage_options);
    test_msgs::msg::BasicTypes msg;
    writer.write(msg, "testtopic", rclcpp::Time{});
  }

[MichaelOrlov]

@Barry-Xu-2018 I made one more round of review.
We are certainly getting close to the finish, but tests still need some "love".
In the current state, almost all newly added tests tend to be flaky and written in non-deterministic way and rely on the ambient delays between critical operations.
I decided to speed up our review and development process and contribute to the tests stability.
I've made a PR Barry-Xu-2018#1 with base to this branch.
Please review my changes and if you don't have any concerns or requests for changes please feel free to merge on your branch.
cc: @fujitatomoya

[Barry-Xu-2018]

@MichaelOrlov

We are certainly getting close to the finish, but tests still need some "love".
In the current state, almost all newly added tests tend to be flaky and written in non-deterministic way and rely on the ambient delays between critical operations.
I decided to speed up our review and development process and contribute to the tests stability.
I've made a PR Barry-Xu-2018#1 with base to this branch.
Please review my changes and if you don't have any concerns or requests for changes please feel free to merge on your branch.

Thank you for your assistance.

[Barry-Xu-2018]

Do rebase since there are conflicts with rolling branch.

[fujitatomoya]

because of ros2/rmw#357, we cannot tell these messages at all, right? is this the current constraints or do we have work-around for that?

[Barry-Xu-2018]

Yes.
From the code perspective, we don't have a workaround.
We have to request that users do not enable service and client introspection simultaneously as a workaround.

[MichaelOrlov]

@Barry-Xu-2018 Wouldn't you mind creating a follow-up issue to not forget to change behavior when ros2/rmw#357 will be resolved?

[fujitatomoya]

@Barry-Xu-2018 i think we are getting closer on this, well tested i think. there are a few need-to-fix comments, please have a look at my comments.

[fujitatomoya]

I will take a look at #1481 after this one has been merged. i think we need to rebase #1481 once this one is merged.

[fujitatomoya]

@Barry-Xu-2018 thanks for the comments. i do not have further comments, lgtm with green CI.
CC: @MichaelOrlov

[MichaelOrlov]

@Barry-Xu-2018 @fujitatomoya I need to recap what has been changed recently and what is yet unclear or unresolved from my questions.

[MichaelOrlov]

@Barry-Xu-2018 I made one more round of review.
I have a few major concerns:

1. As per my comment in the Implement service recording and display info about recorded services #1480 (comment) The service_in_list(..) function is redundant and introduces some confusion and potentially could be a source of errors in the future.
2. As per my comment in the Implement service recording and display info about recorded services #1480 (comment) Relying on the assumption that embedded service dependencies will be always in the MSG files is totally wrong!
   This is a valid scenario when the service's type dependencies could be in IDL files. Need to properly handle this case and correctly detect and save in what encoding message definition is stored for further proper decoding on playback.
3. There are some leftovers after rebase. Need to delete rosbag2_transport/qos.cpp. Since it was moved to the rosbag2_storage package and there are some delta from prior PRs. Delta from the rosbag2_transport/qos.cpp shall be disregarded as per our common agreement here Implement service recording and display info about recorded services #1480 (comment)

To avoid long ping-pong in review and speed up process I've made all fixes which I think shall be done for this PR in the separate branch and prepared a PR Barry-Xu-2018#2. Please review it. If you have no concerns or comments feel free to merge it on your branch or cherry-pick my commits.

[Barry-Xu-2018]

@MichaelOrlov

Thank you for your comments.

As per my comment in the #1480 (comment) The service_in_list(..) function is redundant and introduces some confusion and potentially could be a source of errors in the future.

Okay. The comparison is actually based on the topic name.

As per my comment in the #1480 (comment) Relying on the assumption that embedded service dependencies will be always in the MSG files is totally wrong!
This is a valid scenario when the service's type dependencies could be in IDL files. Need to properly handle this case and correctly detect and save in what encoding message definition is stored for further proper decoding on playback.

Yes, I didn't consider this scenario in my implementation.
Thank you for helping to improve the code.

There are some leftovers after rebase. Need to delete rosbag2_transport/qos.cpp. Since it was moved to the rosbag2_storage package and there are some delta from prior PRs. Delta from the rosbag2_transport/qos.cpp shall be disregarded as per our common agreement here #1480 (comment)

Yes.

To avoid long ping-pong in review and speed up process I've made all fixes which I think shall be done for this PR in the separate branch and prepared a PR Barry-Xu-2018#2. Please review it. If you have no concerns or comments feel free to merge it on your branch or cherry-pick my commits.

After review, LGTM.
I merged it. Thank you.

[MichaelOrlov]

@Barry-Xu-2018 Thank you, now LGTM with green CI.
Also please fix DCO. There is one missing signature for one commit.

[MichaelOrlov]

Re-running CI after addressing test failures and merge conflicts after rebasing.
Gist: https://gist.githubusercontent.com/MichaelOrlov/23b0b25def56ecd75ef2b9040aa28474/raw/ac6760b11c9c612a653f40a9106bae4e5f5d922d/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13043

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

@Barry-Xu-2018 Windows build fails with error message:

rosbag2_format_output.vcxproj -> C:\ci\ws\build\rosbag2_py\Release\rosbag2_format_output.dll

  Building Custom Rule C:/ci/ws/src/Barry-Xu-2018/rosbag2/rosbag2_py/CMakeLists.txt

  _info.cpp

LINK : fatal error LNK1181: cannot open input file 'Release\rosbag2_format_output.lib' [C:\ci\ws\build\rosbag2_py\_info.vcxproj]

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2023-12-14/35153/1

[MichaelOrlov]

Re-run CI after an attempt to fix Windows build errors by adding visibility control to the new CPP library

BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13062

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@Barry-Xu-2018 @MichaelOrlov thank you very much! 🤞🤞🤞

[MichaelOrlov]

@Barry-Xu-2018 Sorry the Windows CI is still unhappy.
There are warnings which need to be addressed.

format_bag_metadata.cpp:229	[C4273]  'format_bag_meta_data': inconsistent dll linkage

and the same warning for the format_service_info

But before start fixing those warnings, I have a question. Why do you need those rosbag2_format_output library at all?
I would just use a regular includes in the _storage.cpp and _info.cpp since all of these references and dependencies are in the scope of the same package, creating a separate library looks like overengineering IMO.
Or maybe I am missing some valuable point?

[MichaelOrlov]

@Barry-Xu-2018 I've made a quick prototype and it seems the code compiles without errors and warnings without rosbag2_format_output library. All tests on my local setup are passing.
See my PR Barry-Xu-2018#4

Please note I've put newly exposed API under the rosbag2_py namespace. If you decided to go with option to keep rosbag2_format_output library will need to do the same and put header files under the separate include folder.

[Barry-Xu-2018]

@MichaelOrlov MichaelOrlov

But before start fixing those warnings, I have a question. Why do you need those rosbag2_format_output library at all?
I would just use a regular includes in the _storage.cpp and _info.cpp since all of these references and dependencies are in the scope of the same package, creating a separate library looks like overengineering IMO.
Or maybe I am missing some valuable point?

As you changed, the same code format_bag_metadata.cpp are included in _storage and _info.

pybind11_add_module(_storage SHARED
  src/rosbag2_py/_storage.cpp
  src/rosbag2_py/format_bag_metadata.cpp
)
...
pybind11_add_module(_info SHARED
  src/rosbag2_py/_info.cpp
  src/rosbag2_py/format_bag_metadata.cpp
  src/rosbag2_py/format_service_info.cpp
)

I considered this approach at the beginning, which would be simpler. However, it would result in duplicate code being compiled into different libraries, which could raise questions during review. So, I decided to compile this part of the code into libraries.
Based on your suggested modifications, the simpler approach seems like the better choice. I agree with that.

[Barry-Xu-2018]

@MichaelOrlov

I've made a quick prototype and it seems the code compiles without errors and warnings without rosbag2_format_output library. All tests on my local setup are passing.
See my PR Barry-Xu-2018#4

Thank you.
There are minor comments. Please look at Barry-Xu-2018#4

[Barry-Xu-2018]

@MichaelOrlov

Barry-Xu-2018#4 has been merged.
Could you run CI again ?

[MichaelOrlov]

Re-run CI after removing rosbag2_format_output cpp library and deleting visibility_control.hpp file

BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13068

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

@Barry-Xu-2018 @fujitatomoya Thank you for your initiative and for driving this large PR.
The PR has finally been merged. Yeah, we made it! 🎆
ROS 2 and rosbag2 became even better than they were before with this PR.
The ability to record service events for further service calls introspection will be a very valuable feature in the future for many developers.
We can count it as a good Christmas gift 🎁
Merry Christmas and Happy Holidays.

[Barry-Xu-2018]

@MichaelOrlov

Merry Christmas and Enjoy your holiday 🎄 🎇 !
I will start to rebase PR #1481.