RSDK-11032: Improved resource logging #535
Conversation
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
The new facility looks nice with a few small questions, but I'm a little puzzled about the choice to rewrite in favor of VIAM_RESOURCE_LOG_THIS systematically.
| ProtoStruct SineWaveAudioIn::do_command(const ProtoStruct& command) { | ||
| for (const auto& entry : command) { | ||
| VIAM_RESOURCE_LOG(info) << "Command entry " << entry.first; | ||
| VIAM_RESOURCE_LOG_THIS(info) << "Command entry " << entry.first; |
There was a problem hiding this comment.
If you have the preprocessor overload thing in place, why switch these and the others in this review to the explicit _THIS form? Wouldn't it be better to either 1) leave them as they are, to demonstrate that the fallback works, or 2) convert them to the new form, to indicate the preferred approach with the new facility?
There was a problem hiding this comment.
(for posterity, doing some offline bikeshedding/discussing to decide if we just nuke the _THIS macro entirely)
|
|
||
| VIAM_RESOURCE_LOG(info) << "Generating sine wave: " << frequency_ << "Hz, " << duration_seconds | ||
| << "s, " << num_chunks << " chunks"; | ||
| VIAM_RESOURCE_LOG_THIS(info) << "Generating sine wave: " << frequency_ << "Hz, " |
There was a problem hiding this comment.
Would VIAM_RESOURCE_LOG(*this be even better?
There was a problem hiding this comment.
I think that's how I'd like to have done it in hindsight
| /// VIAM_RESOURCE_LOG(my_arm, info) << "Message relating to my_arm resource"; | ||
| /// in which case the log message will be associated with the log source of that resource, allowing | ||
| /// resource-level log filtering. | ||
| #define VIAM_RESOURCE_LOG(...) BOOST_PP_OVERLOAD(VIAM_RESOURCE_LOG_IMPL_, __VA_ARGS__)(__VA_ARGS__) |
There was a problem hiding this comment.
I'm always surprised at the cool things you can do with BOOST_PP.
I used it to figure out how to make a reusable facility for import/export macros: https://stackoverflow.com/questions/40944257/can-boost-pp-defined-be-implemented
There was a problem hiding this comment.
really clever use case!
| Name get_resource_name(const std::string& type) const; | ||
|
|
||
| LogSource logger_; | ||
| mutable LogSource logger_; |
There was a problem hiding this comment.
It isn't immediately apparent to me why this needs to become mutable
There was a problem hiding this comment.
I made it mutable because I would like for one to be able to log from within void uses_resource(const Resource&)
stuqdog
left a comment
stuqdog
left a comment
There was a problem hiding this comment.
looks reasonable to me! though I admit I'm slightly out of my depth on some of this.
3 participants
Allows associating log messages with a resource from contexts other than member function implementation. You can call
VIAM_RESOURCE_LOG(severity)from within a member function definition as before, or, using macro overloading, callVIAM_RESOURCE_LOG(resource_ref, severity)from other contexts. IntroducesVIAM_RESOURCE_LOG_THISas an alternative spelling of the single argument version, which should maybe be preferred.One possible point of confusion is that if the user passes
--log-level=debugon the command line, they might expect that to set the default log level for resources to debug as well. I'm open to including that in this PR, but also I think that function is already kind of in a sad state of affairs given that it doesn't parse any other log levels