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 throttled logging when send buffer is full #128

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Jan 4, 2023

Public-Facing Changes

  • Add logging when a connection's send buffer is full

Description
Logs a warning when a connection's send buffer limit has been reached. To avoid log spam, we log this message at max. every 2.5 seconds.

// arguments to pass to the function.
#define FOXGLOVE_DEBOUNCE(f, ms, ...) \
do { \
static auto last_call_##__LINE__ = std::chrono::system_clock::now(); \
Copy link
Member

Choose a reason for hiding this comment

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

I don't think __LINE__ is really necessary since the variable is scoped within the do{}while() anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without __LINE__, if you use the macro in different places, they would interfere since they would use the same static variable

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@achim-k that would be true if this was a function, but it's a macro. Each instance of FOXGLOVE_DEBOUNCE will copy-paste a new variable definition inside a do { } block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, you are right! Will change it

Comment on lines 834 to 836
[this](websocketpp::log::level level, const std::string& msg) {
_server.get_elog().write(level, msg);
},
Copy link
Member

Choose a reason for hiding this comment

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

why not just [&] { _server.get_elog().write(WARNING, "..."); } ?

Copy link
Collaborator Author

@achim-k achim-k Jan 4, 2023

Choose a reason for hiding this comment

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

That's what I had initially, this gave however errors for melodic:

ISO C++11 requires at least one argument for the ...'' in a variadic macro

Edit: https://github.com/foxglove/ros-foxglove-bridge/actions/runs/3840300446/jobs/6539184886

Copy link
Member

Choose a reason for hiding this comment

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

I suppose you could just not allow variadic args.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove it

@achim-k achim-k merged commit c125dd2 into main Jan 4, 2023
@achim-k achim-k deleted the achim/log_when_buffer_full branch January 4, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants