gazebosim / gazebosim/sdformat

Explicitly delimit EOM for logging

Open
#820 11 comments 0 reactions 1 assignee Claimed by @marcoag View on GitHub
enhancement
Dominant language
C++
Stars
216
Forks
125
Avg merge
1d 14h
Merged PRs (30d)
14

Description

## Desired behavior

The Console (i.e., logging) abstraction in libsdformat currently follows the semantics of something like `std::cerr` -- a simple text stream where the call sites conclude each call with a newline (`sdferr << ... << "\n";`). Even if we switch to use the ignition-common console per #351, it still currently embodies the same abstraction.

However, in a lot of production code, log message telemetry is not just a dedicated console read by human eyes -- it is recorded in a structured form, sent over the network, and/or otherwise treated on a _message_ basis rather a _stream_ basis.

My request here is to change the Console to use the same semantics -- where there is a clear "end of message" demarcation.

Several C++ logging frameworks (e.g., log4cpp) do this by using the _logger lifetime_ to conclude the message. My patch file in https://github.com/RobotLocomotion/drake/pull/16348 shows an example of this. The `Console::ColorMsg` function returns a short-lived stream by-value, instead of a long-lived stream by-reference. When the statement ends, the temporary is destroyed, and in its destructor the message is posted.

Another technique is what `spdlog` does -- the user makes a single function call to log, passing both the format string and all of the arguments all at once. This would be a more significant rewrite of the calling-code than the stream lifetime change, but would be a better choice in case the `Console` needed to be used from languages beyond just C++.

## Alternatives considered

A different design would be to keep the streaming API unchanged, but to have the _implementation_ monitor the character buffer insertions for newlines, and replace all newlines with EOM / log posting. I am not a fan of that design, because it removes the ability for a single log message to have embedded newlines, which I've often found to be useful.

## Additional context

For sdformat in particular, as I've explained in https://github.com/ignitionrobotics/sdformat/issues/334#issuecomment-683985626 the _library_ should never be printing to any console anyway. The calls to parse should have a _structured_ way to report errors, and should be silent on `cerr`. The current design and use of `sdf::Error` gets us most of the way there, but there are still a ton of places in the library code that stream to `sdferr` (or `sdfwarn`, etc.) instead of appending an `sdf::Error`. If we fixed _that_ problem instead, then we could delete the sdformat-specific `Console` class and this whole discussion here is moot (though would still apply to the ignition-common logging API design).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.