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 #14

Closed

Conversation

HannesBachter
Copy link

adds the option to use Logger.log*_throttle
(same as team-vigir/flexbe_behavior_engine#166)

@@ -16,11 +16,15 @@ class Logger(object):

LOGGING_TOPIC = 'flexbe/log'

# max number of items in last logged dict (used for log_throttle)
MAX_LAST_LOGGED_SIZE = 1024
Copy link
Author

@HannesBachter HannesBachter Nov 20, 2023

Choose a reason for hiding this comment

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

Any good idea on how to make that size configurable?

Logger.log(text, severity)
Logger._last_logged.update({log_id: rospy.Time.now()})

if len(Logger._last_logged) > Logger.MAX_LAST_LOGGED_SIZE:
Copy link
Member

Choose a reason for hiding this comment

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

Once we hit limit this will process every message following.

Should we consider throwing away the oldest half of messages, otherwise i fear this limit will become a limiting factor given need to sort and pop when adding a single new message.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, although this would only occur under the circumstance that we have more than 1024 log_throttles and it would only happen when a changing message is logged.
But I agree, that we don't want to have that overhead. I'll change the clearing to a percentage to the MAX_LAST_LOGGED_SIZE 👍

@HannesBachter
Copy link
Author

@dcconner anything else you need here?

@dcconner
Copy link
Member

Just time. Traveling now over break, but I hope to get some time after new years

@HannesBachter
Copy link
Author

@dcconner sorry to bother you, but I just realised, that the PR is still open :)

@dcconner
Copy link
Member

" some time after new years" is a broad statement ;-) Unfortunately I have not had time to test this myself as I'm teaching a new course. Our semester ends in early May, and I do intend to devote time in May to getting code ready for an Iron update and new release. So, if I haven't merged this by the end of May then you should yell at me.

@dcconner
Copy link
Member

@HannesBachter Do you have a simple test behavior for this change?

dcconner pushed a commit that referenced this pull request May 1, 2024
dcconner pushed a commit that referenced this pull request May 1, 2024
dcconner pushed a commit that referenced this pull request May 1, 2024
dcconner pushed a commit that referenced this pull request May 1, 2024
@dcconner
Copy link
Member

dcconner commented May 1, 2024

incorporated into humble, iron, rolling, and ros2-devel branches

@dcconner dcconner closed this May 1, 2024
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