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

Warning Message Intervals for canTransform #663

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

CursedRock17
Copy link
Contributor

@CursedRock17 CursedRock17 commented Mar 17, 2024

This PR is meant to prevent the spam of messages from the method canTransform, while still remaining security, preventing an API break, and reducing load. There are a slew of issues and pull requests related to this problem:

Issues Pull Requests
#358 #395
#405 #556
#396 #491

With all those pull requests looking very similar. Though, since the merging of #655 we have access to the macro RCUTILS_LOG_WARN_THROTTLE which solves the caveat of

Unfortunately I don't think that console_bridge directly supports a THROTTLED version of the logging macros which would likely be the simplest solution to slow it down.

By using a throttled version we can release warning messages every 5000 ms (currently the default), that way the messge can't be ignored by the user, but they won't annihilate the terminal.

@ahcorde
Copy link
Contributor

ahcorde commented Mar 18, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor

ahcorde commented Mar 18, 2024

@tfoote or @clalancette fix looks good to me, but maybe there is a reason to continiously send these amount of messages.

Thoughts?

@tfoote tfoote self-requested a review March 18, 2024 09:39
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

@CursedRock17
Copy link
Contributor Author

It seems the main error is in rviz_common: these two functions

They are overrides of canTransform which currently doesn't have the warning_interval argument, so the code fails. I updated all the Buffer classes' canTransform methods within this repository so the code is good here, but any other repository overriding these classes won't compile. Does this still count as an API break? I originally put in a default argument to prevent any API usage of canTransform from breaking, which is still technically the case as calling the function will continue to work. Also, it's not like the Buffer methods higher on the inheritance chain can go without this, since BufferCore (Where the warning message occurs) overrides those methods. This seems to leaves us with three options

  1. Leave with a broken CI and other broken repositories, then quickly issue a fix if this gets merged
  2. Scrap the function parameter of warning_interval and use a certain local value for the Throttle value
  3. Scrap the function parameter of warning_interval and use a private value with setter/getter functions. It would just be a bit awkward not being able to use that value for anything else

The nice thing about removing the function parameter would be that functions like BufferClient::canTransform won't have to worry about not using warning_interval.

It's also a good time to ask if a default value of 5000ms (5 secs) is alright, I figured anything in the range of [2,8] was ok.

@CursedRock17
Copy link
Contributor Author

I decided to go with option 2 as I felt option 3 was going to be more awkward, plus various other repositories set local values for THROTTLED logs in their APIs - NAV2, image_pipeline. Thus, I decreased the throttle time from 5 seconds to 2.5 seconds as it would be more appropriate in various scenarios.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

tf2/include/tf2/buffer_core.h Outdated Show resolved Hide resolved
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
Signed-off-by: CursedRock17 <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor

ahcorde commented Apr 4, 2024

@ros-pull-request-builder retest this please

@ahcorde ahcorde merged commit f63c7ae into ros2:rolling Apr 4, 2024
1 of 2 checks passed
@CursedRock17
Copy link
Contributor Author

Real quick, does this PR resolve any of the pull requests or issues in the table in the original description?

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