ros2 / geometry2

A set of ROS packages for keeping track of coordinate transforms.
BSD 3-Clause "New" or "Revised" License
110 stars 193 forks source link

Warning Message Intervals for canTransform #663

Closed CursedRock17 closed 3 months ago

CursedRock17 commented 3 months ago

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 commented 3 months ago
ahcorde commented 3 months ago

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

Thoughts?

CursedRock17 commented 3 months ago

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 commented 3 months ago

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.

ahcorde commented 3 months ago

@ros-pull-request-builder retest this please

CursedRock17 commented 3 months ago

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