Programmatic logger severity thresholds #57

dhood · 2017-10-10T19:57:27Z

~~still in progress~~ description below.

Can't stop the log calls from being called, so handle it gracefully

This reverts commit 182b133.

It was the absence of a name; we don't prepend it to other names.

dhood · 2017-10-19T16:49:30Z

Latest CI including ros2/rclpy#130:

Linux
Linux-aarch64
macOS
Windows

dhood · 2017-10-20T23:10:55Z

this is ready for another round of review @ros2/team. everything in my previous summary is still valid (scope, points potentially up for discussion)

dirk-thomas · 2017-10-20T23:30:21Z

src/logging.c

+  [RCUTILS_LOG_SEVERITY_WARN] = "WARN",
+  [RCUTILS_LOG_SEVERITY_ERROR] = "ERROR",
+  [RCUTILS_LOG_SEVERITY_FATAL] = "FATAL",
+};


How are the other entries initialized?

automatically as null, there's a check for null after retrieving the values at: https://github.com/ros2/rcutils/pull/57/files/ab249daaef9556c9aa2a2b79bc0ccb4a98829b61#diff-f909d0b1a298531f76132943dc55f86aR190

dirk-thomas · 2017-10-20T23:31:57Z

src/logging.c

  return g_rcutils_logging_output_handler;
 }

 void rcutils_logging_set_output_handler(rcutils_logging_output_handler_t function)
 {
-  g_rcutils_logging_output_handler = function;
+  RCUTILS_LOGGING_AUTOINIT
+    g_rcutils_logging_output_handler = function;


Indentation seems to be off?

Same below.

yeah this is what uncrustify wanted: I'll disable what's making it complain

I allowed them to be independent as an arbitrary decision but equating them is needed to facilitate an empty-named root logger concept in wrappers

dhood · 2017-11-02T00:42:56Z

Ready for review. What's changed since the last summary is that now the empty-named logger acts as if it's the root logger: changing the severity threshold for the empty-named logger changes the default severity. This was done in order to facilitate an empty root logger in ros2/rclpy#132 (comment). The default logger's severity is still stored as a global variable, instead of in the severity map, to facilitate global configuration even when the severity map can't be used for some reason.

dhood · 2017-11-03T22:46:04Z

@ros2/team if anyone has an idea for how I can help this be easier to review I'm open to suggestions. If it would help someone, I can provide descriptions, discussions, or break it into several PRs that build on top of each other.

wjwwood · 2017-11-03T23:29:11Z

include/rcutils/find.h

+ * \param[in] delimiter the character to search for
+ * \returns the index of the first occurence of the delimiter if found, or
+ * \returns `string_length` if the delimiter is not found, or
+ * \returns `0` for invalid arguments


What if the delimiter is at index 0? How do you know the difference between that and invalid arguments?

Same question for findn version.

@clalancette also raised this question in #57 (comment); the conclusion was that it will be addressed as followup #58

wjwwood · 2017-11-03T23:41:00Z

include/rcutils/find.h

+ * \param[in] delimiter the character to search for
+ * \returns the index of the last occurence of the delimiter if found, or
+ * \returns `string_length` if the delimiter is not found, or
+ * \returns `0` for invalid arguments


Same question, what if delimiter is at 0 only.

wjwwood · 2017-11-04T00:18:26Z

include/rcutils/logging.h

+ * \param name The name of the logger, must be null terminated c string or NULL.
+ * \param severity The severity level.
+ *
+ * \return True if the logger is enabled for the severity; false otherwise.


nitpick: True is uppercase and false is lower, lower is correct in C++

thanks, I was capitalising the sentence. fixed in 3024667

wjwwood · 2017-11-04T00:34:13Z

src/logging.c

+    if (!rcutils_allocator_is_valid(&allocator)) {
+      fprintf(
+        stderr,
+        "Provided allocator is invalid. Using the default allocator.\n");


Since you're returning an error code should you use RCUTILS_SET_ERROR_MSG?

Same below.

dhood · 2017-11-07T20:34:47Z

@wjwwood could you please review the recent changes regarding your suggestion to use RCUTILS_SET_ERROR_MSG:

1594562 adds setting of error messages where error codes are being returned (newlines removed in c8e5725)
I saw a quote of your comment in Accumulate DDS errors during cleanup and return error code from wait() function rmw_connext#265, so 371c8bf logs when the error message is being overwritten (when it is detected) and 808a64b returns an RCUTILS_RET_MULTIPLE_ERRORS rc in the event multiple issues occur.
- In the case that the string_map has issues at 371c8bf#diff-f909d0b1a298531f76132943dc55f86aL102, the overwriting will be logged by rcutils underneath us; the return code will still reflect multiple errors.

dhood · 2017-11-09T19:02:32Z

I've moved the commits related to RCUTILS_SET_ERROR_MSG into a separate PR #65 so that I can merge what has been approved here, and then we can more easily discuss the new commits.

This is CI for the approved changes here and the approved ros2/rclpy#132. I'll merge.

Linux
Linux-aarch64 (known flaky test)
macOS (known flaky test)
Windows

dhood added the in progress Actively being worked on (Kanban column) label Oct 10, 2017

dhood self-assigned this Oct 10, 2017

dhood added 28 commits October 13, 2017 12:08

WIP specify logger severity by name

e4cd241

Add a fini to destroy severity map

adbff89

wip cont

86b4c92

Add is_enabled_for concept

3cd2460

People won't override is_enabled_for

46b7462

fini -> shutdown

28f0903

Fallback to no configurability if map errors

44f40c4

Can't stop the log calls from being called, so handle it gracefully

Hierarchy will require distinction between set and unset levels

2211391

More error handling

17c0076

WIP logger hierarchy

7e71c2c

Add a non-null-terminated get_severity to avoid strdup

4c160bd

lint

0d8624d

Add tests for effective threshold

085343c

Docblock update

134e52f

Additional doc fixup

b6a8b2a

Replace global severity check in rcutils_log

593d5b2

Remove debug prints

9388d1f

get_severity_threshold -> get_global_severity_threshold

3a8820d

Revert "get_severity_threshold -> get_global_severity_threshold"

2fc3aa1

This reverts commit 182b133.

Use RCUTILS_LOGGING_ROOT_LOGGER_NAME

849ecfd

Remove the notion of global severity threshold

8f77dc6

Add initialize() where allocator's specified

cbae039

safer logic

dff0479

RCUTILS_LOGGING_ROOT_LOGGER_NAME wasn't really a root logger.

7bcff94

It was the absence of a name; we don't prepend it to other names.

root severity threshold -> default severity threshold

d4c7c28

Distinguish between nameless log calls and those with empty name

2f2b9de

The map will allocate space for a new string regardless

854b5cd

Add more documentation

0080d54

dhood added 2 commits October 18, 2017 18:58

Use new severity level name array

939a435

Remove 'unknown error' reference for key_exists return

27bde82

dhood mentioned this pull request Oct 19, 2017

Updates for global severity threshold -> default severity threshold ros2/rclpy#130

Closed

dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 19, 2017

dhood added 2 commits October 19, 2017 13:25

logger_effective_threshold -> logger_effective_severity_threshold

360e1b1

is_enabled_for -> logger_is_enabled_for

a74a4fc

clalancette mentioned this pull request Oct 20, 2017

find() and find_last() functions return ambiguous value #58

Closed

spelling

ab249da

dhood mentioned this pull request Oct 20, 2017

Named logger config + logger hierarchy ros2/rclpy#132

Merged

dirk-thomas reviewed Oct 20, 2017

View reviewed changes

Prevent uncrustify from making indents with LOGGIN_AUTOINIT

a78c886

dhood mentioned this pull request Oct 23, 2017

Prototype for using logging macros ros2/demos#184

Closed

dhood added 2 commits October 24, 2017 11:20

Equate the empty logger name to the default

e86c6c4

I allowed them to be independent as an arbitrary decision but equating them is needed to facilitate an empty-named root logger concept in wrappers

add comment

8e755b7

wjwwood approved these changes Nov 4, 2017

View reviewed changes

True -> true

3024667

dhood mentioned this pull request Nov 9, 2017

Use RCUTILS_SET_ERROR_MSG in logging config #65

Merged

dhood force-pushed the logging_config branch from e080db6 to 3024667 Compare November 9, 2017 18:59

dhood merged commit b981951 into master Nov 9, 2017

dhood removed the in review Waiting for review (Kanban column) label Nov 9, 2017

dhood deleted the logging_config branch November 9, 2017 19:25

mikaelarguedas mentioned this pull request Dec 14, 2017

out-of-bounds read issue #83

Merged

tfoote mentioned this pull request Nov 15, 2018

Adding a message type for Log messages that will be sent out over the… ros2/rcl_interfaces#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Programmatic logger severity thresholds #57

Programmatic logger severity thresholds #57

dhood commented Oct 10, 2017 •

edited

Loading

dhood commented Oct 19, 2017

dhood commented Oct 20, 2017

dirk-thomas Oct 20, 2017

dhood Oct 20, 2017

dirk-thomas Oct 20, 2017

dhood Oct 20, 2017

dhood Oct 20, 2017

dhood commented Nov 2, 2017

dhood commented Nov 3, 2017

wjwwood Nov 3, 2017

wjwwood Nov 3, 2017

dhood Nov 7, 2017 •

edited

Loading

wjwwood Nov 3, 2017

wjwwood Nov 4, 2017

dhood Nov 7, 2017

wjwwood Nov 4, 2017

wjwwood Nov 4, 2017

dhood commented Nov 7, 2017

dhood commented Nov 9, 2017 •

edited

Loading

Programmatic logger severity thresholds #57

Programmatic logger severity thresholds #57

Conversation

dhood commented Oct 10, 2017 • edited Loading

dhood commented Oct 19, 2017

dhood commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Nov 2, 2017

dhood commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Nov 7, 2017

dhood commented Nov 9, 2017 • edited Loading

dhood commented Oct 10, 2017 •

edited

Loading

dhood Nov 7, 2017 •

edited

Loading

dhood commented Nov 9, 2017 •

edited

Loading