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)
There was a problem hiding this comment.
My vote is for giving it a try and as long as it works without surprises, do it in this review.
|
|
||
| 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
There was a problem hiding this comment.
Changing these macros is annoying; my instinct is to pursue this in this review.
| 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&)
There was a problem hiding this comment.
Ah, OK. I guess I sort of expected that as a self-synchronizing (presumably, or how does our logging work at all) that it would work as a const object. But, if not, I agree the mutable is necessary.
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