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

fix: check_ctf warning message #308

Merged
merged 5 commits into from
Jun 29, 2023
Merged

Conversation

ymski
Copy link
Contributor

@ymski ymski commented Jun 27, 2023

Description

After #PR296, the warning given by ros2 caret check_ctf was not appropriate.

CARET uses the original tracepoint definition to calculate latency.
ros2 caret check_ctf is a checking command for the above tracepoints.
If there were no original tracepoints, we output the warning, but the following warning was also output even when the analysis could be carried out normally.

Failed to find trace point added by caret-rclcpp.
To check whether binary are built with caret-rclcpp,
run CARET CLI : ros2 caret check_caret_rclcpp.

The warnings are erroneously output in the following cases:

In the following cases, analysis could be performed but a warning was output.

  • Built with caret-rclcpp, but the application doesn't have intra-process communication

Related links

Notes for reviewers

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR has been properly tested.
  • The PR has been reviewed.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • (Optional) The PR has been properly tested with CARET_report verification.
  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: yamasaki <114902604+ymski@users.noreply.github.com>
Signed-off-by: yamasaki <114902604+ymski@users.noreply.github.com>
Signed-off-by: yamasaki <114902604+ymski@users.noreply.github.com>
@ymski ymski changed the title fix: fix check_ctf warning message fix: check_ctf warning message Jun 28, 2023
@ymski ymski marked this pull request as ready for review June 28, 2023 03:33
@ymski ymski requested a review from isp-uetsuki June 28, 2023 03:33
src/caret_analyze/infra/lttng/event_counter.py Outdated Show resolved Hide resolved
src/caret_analyze/infra/lttng/event_counter.py Outdated Show resolved Hide resolved
ymski and others added 2 commits June 28, 2023 17:56
Co-authored-by: isp-uetsuki <35490433+isp-uetsuki@users.noreply.github.com>
Signed-off-by: yamasaki <114902604+ymski@users.noreply.github.com>
@ymski ymski requested a review from isp-uetsuki June 28, 2023 09:10
Copy link
Contributor

@isp-uetsuki isp-uetsuki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@takeshi-iwanari takeshi-iwanari left a comment

Choose a reason for hiding this comment

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

I attached how warning is output for each version and each condition. Everything looks okay to me. Could you also confirm if they are as you intended?
If it's okay, we can merge it.

CARET_trace TraceData caret/rclcpp LD_PRELOAD Result (Displayed warning) Result Note
V0.4.11 caret_demos (no intra comm) y y No Warning OK w/ v0.4.11: Failed to find trace point added by caret-rclcpp. The original issue is fixed
V0.4.11 caret_demos (no intra comm) y n Failed to find trace point added by LD_PRELOAD OK
V0.4.11 caret_demos (no intra comm) n y No Warning OK w/ v0.4.11: Failed to find trace point added by caret-rclcpp. Behavior changed but it's okay
V0.4.11 caret_demos (no intra comm) n n Failed to find trace point added by LD_PRELOAD OK
V0.4.11 Autoware (has intra comm) y y No Warning OK w/ v0.4.11: no warning because it has intra comm
V0.4.11 Autoware (has intra comm) y n Failed to find trace point added by LD_PRELOAD OK
V0.4.11 Autoware (has intra comm) n y Failed to find trace points added by caret-rclcpp. If the application doesn't have any intra communication, please ignore this message. To check whether binary are built with caret-rclcpp, run CARET CLI : ros2 caret check_caret_rclcpp. OK w/ v0.4.11: Failed to find trace point added by caret-rclcpp.
V0.4.11 Autoware (has intra comm) n n Failed to find trace point added by LD_PRELOAD OK
V0.4.11 Autoware (has intra comm)+light y y No Warning OK w/ v0.4.11: no warning because it has intra comm
V0.4.11 Autoware (has intra comm)+light y n Failed to find trace point added by LD_PRELOAD OK
V0.4.11 Autoware (has intra comm)+light n y Failed to find trace points added by caret-rclcpp. If the application doesn't have any intra communication, please ignore this message. To check whether binary are built with caret-rclcpp, run CARET CLI : ros2 caret check_caret_rclcpp. OK w/ v0.4.11: Failed to find trace point added by caret-rclcpp.
V0.4.11 Autoware (has intra comm)+light n n Failed to find trace point added by LD_PRELOAD OK
V0.4.8 caret_demos (no intra comm) y y No Warning OK
V0.4.8 caret_demos (no intra comm) y n Failed to find trace point added by LD_PRELOAD OK
V0.4.8 caret_demos (no intra comm) n y Failed to find trace points added by caret-rclcpp. To check whether binary are built with caret-rclcpp, run CARET CLI : ros2 caret check_caret_rclcpp. OK
V0.4.8 caret_demos (no intra comm) n n Failed to find trace point added by LD_PRELOAD OK
V0.4.8 Autoware (has intra comm)+light y y No Warning OK w/ v0.4.11: no warning because it has intra comm
V0.4.8 Autoware (has intra comm)+light y n Failed to find trace point added by LD_PRELOAD OK
V0.4.8 Autoware (has intra comm)+light n y Failed to find trace points added by caret-rclcpp. If the application doesn't have any intra communication, please ignore this message OK w/ v0.4.11: Failed to find trace point added by caret-rclcpp. To check whether binary are built with caret-rclcpp,run CARET CLI
V0.4.8 Autoware (has intra comm)+light n n Failed to find trace point added by LD_PRELOAD OK

@ymski
Copy link
Contributor Author

ymski commented Jun 29, 2023

@takeshi-iwanari

Thank you for validating the various cases.
The table you provided appears to be working as intended.

@ymski ymski merged commit 2146d75 into tier4:main Jun 29, 2023
13 checks passed
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.

3 participants