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

Add throttle option for logging #166

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

HannesBachter
Copy link

adds the option to use Logger.log*_throttle

if not log_id in Logger._last_logged.keys() or \
rospy.Time.now().to_sec() - Logger._last_logged[log_id].to_sec() > period:
Logger.log(text, severity)
Logger._last_logged.update({log_id: rospy.Time.now()})
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding! Would you see a way that entries can also be removed again eventually so that this dict can't grow infinitely?

Copy link
Author

@HannesBachter HannesBachter Mar 28, 2023

Choose a reason for hiding this comment

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

The update function would only update the entry within the dict (whose key is a combination of the logger level and the text) - so the dictionary would only have the max length of the number of Logger.log*_throttles
This idea works only until the text within the log is not static, and in the worst case changing for every log (e.g. a timestamp).
As a safety-net I could add the clearing of the dict from all entries that are older than e.g. one hour (as the entries are {log_id: timestamp})?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be sufficient as a safety-net to just check whether the number of dict entries exceeds a certain limit and then remove entries. As long as the limit is not exceeded, it's probably fine to keep old entries.
As a further idea, I would suggest to log a warning message if the size limit is reached because, as you pointed out, this would likely be caused by improper usage of the throttle such as a non-static messages.

@dcconner
Copy link
Member

Would it be reasonable to include a limit on dictionary size?
https://itecnote.com/tecnote/python-how-to-limit-the-size-of-a-dictionary/

@HannesBachter
Copy link
Author

@pschillinger ping :)

@dcconner
Copy link
Member

@HannesBachter , with @pschillinger comments incorporated, would you be willing to submit a PR against the Noetic branch at https://github.com/FlexBE/flexbe_behavior_engine . That's where we are maintaining the behavior engine going forward. I'll take care of getting in to the ROS 2 branches. I don't plan to make changes to this old version.

@HannesBachter
Copy link
Author

Opened a new PR here: FlexBE/flexbe_behavior_engine#14

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.

4 participants