-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove repetition of metadata in log messages #2953
Remove repetition of metadata in log messages #2953
Conversation
…dkAssembleAndLog macro
…/amazon-freertos into poc/logging-thread-safety-issue
/bot run checks |
4 similar comments
/bot run checks |
/bot run checks |
/bot run checks |
/bot run checks |
…pied for enqueing and processing in logging task
#endif | ||
|
||
#define SdkLogError( message ) SdkLogErrorC99 message | ||
#define SdkLogErrorC99( format, ... ) vLoggingPrintf( "[ERROR] [%s] [%s:%d] " format "\r\n", LIBRARY_LOG_NAME, FILENAME, __LINE__, ## __VA_ARGS__ ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better to keep the metadata format and metadata as another MACRO similar to how it was. This will have Single source of truth for metadata information.
libraries/logging/README.md
Outdated
@@ -0,0 +1,109 @@ | |||
## Sample Implementation of Logging Interface | |||
|
|||
This library provides reference implementations of the [logging interface](#the-logging-interface) that is used by FreeRTOS libraries and demos to generate log messages. This library provides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that the word "library" will be changed to sample/demo when you move it to demos folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the wording to "sample" now itself :)
@@ -0,0 +1,109 @@ | |||
## Sample Implementation of Logging Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sample Implementation of Logging Interface
This library provides reference implementations of the
logging interface that is used by FreeRTOS libraries
and demos to generate log messages. This library provides implementations for
both ISO C90 standard and ISO C99 standard (with GNU extension) of the logging
interface.
Note: Users can provide their own implementations of the logging interface
to enable logging, if they choose to not use this implementation.
To enable logging using the sample implementation for a FreeRTOS library and/or
demo:
- Configure logging level by defining
LIBRARY_LOG_LEVEL
macro to one of
LOG_NONE
,LOG_ERROR
,LOG_WARN
,LOG_INFO
,LOG_DEBUG
. - If using the ISO C99 (with GNU extension) implementation (which is default),
defineLIBRARY_LOG_NAME
to configure the library name which is prepended to
the log messages. - If your compiler only supports the ISO C90 standard, use the C90
implementation by setting theLOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION
macro to0
.
The logging file include and macro definitions must follow the following order:
- Include
logging_levels.h
. - Define
LIBRARY_LOG_LEVEL
,LIBRARY_LOG_NAME
, and
LOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION
as mentioned above. - Include
logging_stack.h
.
A sample logging configuration for the coreMQTT library which is defined in
core_mqtt_config.h
file:
#include "logging_levels.h"
#ifndef LIBRARY_LOG_NAME
#define LIBRARY_LOG_NAME "MQTT"
#endif
#ifndef LIBRARY_LOG_LEVEL
#define LIBRARY_LOG_LEVEL LOG_INFO
#endif
#include "logging_stack.h"
The above configuration results in the logging messages looking like the following.
Note that the library name ([MQTT]
) and source file location ([core_mqtt.c:1175]
)
are prepended to each log message.
78 1950 [iot_thread] [INFO] [MQTT] [core_mqtt.c:886] Packet received. ReceivedBytes=2.
79 1950 [iot_thread] [INFO] [MQTT] [core_mqtt.c:1162] Ack packet deserialized with result: MQTTSuccess.
80 1951 [iot_thread] [INFO] [MQTT] [core_mqtt.c:1175] State record updated. New state=MQTTPublishDone.
If ISO C90 implementation is used (i.e. LOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION
is defined to 0
), the logging messages look like the following. Note that the
library name and source file location are not present in each log message.
78 1950 [iot_thread] [INFO] Packet received. ReceivedBytes=2.
79 1950 [iot_thread] [INFO] Ack packet deserialized with result: MQTTSuccess.
80 1951 [iot_thread] [INFO] State record updated. New state=MQTTPublishDone.
Using your custom implementation of Logging Interface
The Logging Interface comprises of the following 4 logging macros, listed in
increasing order of verbosity:
LogError
LogWarn
LogInfo
LogDebug
LogError
is only called when there is an error and so is the least verbose,
whereas LogDebug
is called more frequently to provide debug level information
and is therefore, the most verbose.
An implementation of the logging interface needs to define these macros to
map them to their logging implementation. Logging macros use format specifier
and variable number of arguments, just like standard library printf
function
but they use double parenthesis to support ISO C90 which should be taken care
while defining them.
An example call of the logging macros is the following:
LogInfo( ( "This prints an integer %d", 100 ) );
If you have a thread safe printf
function, LogInfo
should be defined like
the following. Notice how one set of parentheses are removed because X
already contains a set of parentheses.
#define LogInfo( X ) printf X
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for providing a concise version of the README.
* @note By default, this configuration is disabled by | ||
*/ | ||
#ifndef LOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION | ||
#define LOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the README I got the impression that the default is 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, will make it clear in the README
/* Add the chosen log level information as prefix for the message. */ | ||
if( pcLevelString != NULL ) | ||
{ | ||
xLength += snprintf( pcPrintString + xLength, configLOGGING_MAX_MESSAGE_LENGTH - xLength, "[%s] ", pcLevelString ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check snprintf
failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assert as negative value is returned by snprintf only for encoding errors.
/bot run checks |
Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>
Problem
Problem 1: Metadata is duplicated in log messages
With the refactored libraries using the new logging macros (
LogError
,LogInfo
,LogWarn
,LogDebug
), the log messages from the libraries contain duplication of task name and tick count metadata which makes the log messages non-readable. The cause of the metadata repetition is that each of theLogXXXX
calls results in 3 calls tovLoggingPrintf
, due to which the metadata is repeated in each call.Here is an example of logs from the
coreMQTT
library when running thecore_mqtt_mutual_auth
demo:Problem 2: Log messages are out-of-order when
LogXXXX
macros are called in multi-threaded environmentThe implementation of
logging_stack.h
makes 3 calls tovLoggingPrintf
which are vulnerable to context-switch before the completion of the 3 calls.Here is an example of mangled logs from the MQTT connection sharing demo that contains log calls from multiple tasks:
Solution
This PR fixes the problem by refactoring the macro layer implementation in
logging_stack.h
to call the logging function (exposed byiot_logging_task.h
) only once instead of 3 times.This PR refactors simplifies the implementation by removing the metadata addition logic for the ISO C90 compliant code. In addition, it also adds support for ISO C99 (with GNU extension) standard to show how extra metadata of source library, file location can be added with simple variadic macro logic.
The library can be configured with either of the C90 or C99 implementations a new macro
LOGGING_ENABLE_METADATA_WITH_C99_AND_GNU_EXTENSION
.After the fix, the above logs from
coreMQTT
library look like: