foxglove / ros-foxglove-bridge

Foxglove WebSocket bridge for ROS 1 and ROS 2
MIT License
168 stars 75 forks source link

Improve Error Reporting and Reduce Log Redundancy #327

Closed rdumas01 closed 4 days ago

rdumas01 commented 1 week ago

Changelog

None

Docs

None

Description

The ROS 1 foxglove_bridge rejects parameter names with . and spams the terminal with redundant error messages.

While it is possible for ROS 1 parameters to have a . in their name, they are not "supposed" to (Character [.] is not valid in Graph Resource Name). For this reason, the foxglove_bridge will keep raising errors for parameter names with .

This PR addresses the error handling part of this issue: an unordered set tracks the encountered invalid parameters to throttle error log messages.

BeforeAfter
The bridge std_out gets periodically spammed with the same error messages until the invalid parameter is removed. In the app, the only visible error message is too vague. error_log_before The bridge std_out only prints errors when invalid parameters are first encountered—one error message per invalid parameter. In the app, the error message lists the invalid parameters. error_log_after
jtbandes commented 1 week ago

Looks good to me, might want to double-check with @achim-k if he has any feedback

defunctzombie commented 1 week ago

If the issue is error logging spam - I'd propose using the throttled variants of these macros:

https://docs.ros.org/en/indigo/api/rosconsole/html/macros__generated_8h.html#a1c4cdfdfd4dcc53e6226f0533a0992b7

This avoids the needs to track the params. If you still want to track the params - maybe some upper bound on number of params tracked? I don't imagine too many problems with this vector growing but as written it is definitely a thing that can grow unbounded if you keep making invalid params.

rdumas01 commented 1 week ago

If the issue is error logging spam - I'd propose using the throttled variants of these macros

I think it's more a dedupe issue than a throttling one, the printing rate is already quite slow (basically it slowly floods the terminal with the same errors, and the more invalid parameters, the more error lines)

maybe some upper bound on number of params tracked?

Yes, good point. I've never had projects with a lot of parameters, is 100 a realistic upper bound?

defunctzombie commented 1 week ago

Yes, good point. I've never had projects with a lot of parameters, is 100 a realistic upper bound?

Could probably make it a thousand - a list of strings is not so insane to store - more that if some weird software does iterate params constantly you don't want that to make the bridge fall over after a few hours. I don't know how realistic of an issue this is but that's what came to mind with adding these to the set.