Add demo for logging + logger config #194

dhood · 2017-11-22T23:03:55Z

Usage:

# start node with logging calls and node that accept logger config service requests
$ ros2 run logging_demo logging_demo_main

# configure logger to debug
$ ros2 service call /config_logger logging_demo/ConfigLogger "{logger_name: 'logger_usage_demo', severity_threshold: DEBUG}"

The LoggerConfig component can be dropped into a running (composition-api-based) system to get config access to the loggers of the process

dhood · 2017-11-28T20:51:54Z

Ready for review.

This demo adds a node using log calls that configures its logger level programmatically.

There's also a component that responds to service requests for changes to the logger severity. This can be added to component-based systems to enable external logger configuration at runtime. I've tried to make it clear in the readme that this isn't the long-term plan for runtime configuration, just something that people might find useful. If we think it sends the wrong message we can just flat out say we only support programmatic configuration instead.

The readme has the usage: https://github.com/ros2/demos/pull/194/files#diff-599b8049677d7c1e29b69de5f3aef287

mikaelarguedas

I looked only at the code and will review the readme and try the instructions in the next pass.

mikaelarguedas · 2017-11-30T19:03:31Z

logging_demo/CMakeLists.txt

+endif()
+
+if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+  add_compile_options(-Wall -Wextra)


Nit: Adding a comment here explaining why we cant use pedantic like the other repos will be useful for book keeping

mikaelarguedas · 2017-11-30T19:04:24Z

logging_demo/CMakeLists.txt

+  macro(tests)
+    set(LOGGING_DEMO_MAIN_EXECUTABLE $<TARGET_FILE:logging_demo_main>)
+    set(EXPECTED_OUTPUT_LOGGING_DEMO_MAIN_DEFAULT_SEVERITY "${CMAKE_CURRENT_SOURCE_DIR}/test/logging_demo_main_default_severity")
+    set(EXPECTED_OUTPUT_LOGGING_DEMO_MAIN_DEBUG_SEVERITY "${CMAKE_CURRENT_SOURCE_DIR}/test/logging_demo_main_debug_severity")


Nit: these two lines look long

mikaelarguedas · 2017-11-30T19:06:57Z

logging_demo/include/logging_demo/logger_config_component.hpp

@@ -0,0 +1,37 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


mikaelarguedas · 2017-11-30T19:07:14Z

logging_demo/include/logging_demo/logger_config_component.hpp

+#define LOGGING_DEMO__LOGGER_CONFIG_COMPONENT_HPP_
+
+#include "logging_demo/visibility_control.h"
+#include "logging_demo/srv/config_logger.hpp"


Nit: include order

mikaelarguedas · 2017-11-30T19:08:52Z

logging_demo/package.xml

+<package format="2">
+  <name>logging_demo</name>
+  <version>0.0.3</version>
+  <description>Examples for composing multiple nodes in a single process.</description>


The description should be updated, maintainer tag as well

do you think it's alright to start this at v0.0.3? figured it's easier for when we bump them all?

yup! will make @nuclearsandwich's blooming easier

All packages in the same repository need to have the same version, so not keeping it in line with the other package's version would actually make it impossible to release :)

ah! ok then it stays!

mikaelarguedas · 2017-11-30T19:19:21Z

logging_demo/CMakeLists.txt

+
+    configure_file(
+      test/test_logging_demo.py.in
+      test_logging_demo${target_suffix}.py.genexp


Out of curiosity what does genexp stands for here?

that it'll be the input to the generate command in the following line to have its generator expressions (above) expanded

ahhh thanks I couldnt figure out what genexp was suppose to abbreviate

mikaelarguedas · 2017-11-30T19:22:15Z

logging_demo/src/logger_usage_component.cpp

+  }
+}
+
+bool divides_into_twelve(size_t val, std::string logger_name)


does this mean that this returns that the value is a divider of 12 ?
(maybe that's my english + math vocabulary here but I had to check the code to see what divides_into_twelve mean). I would have expected this to return (val % 12) == 0 aka is_multiple_of_twelve

yeah it means is_divisor_of_twelve, I'll update the name

mikaelarguedas · 2017-11-30T19:25:02Z

logging_demo/test/test_logging_demo.py.in

+    launch(name, [executable], output_file)
+
+
+def launch(name, cmd, output_file, additional_processes=None):


this could default to [] and avoid adding the logic (additional_processes or []) below

We try to avoid mutable defaults in general

fair point, forgot about that 👍

mikaelarguedas · 2017-11-30T19:26:11Z

logging_demo/test/test_logging_demo.py.in

+
+    assert rc == 0, \
+        "The launch file failed with exit code '" + str(rc) + "'. " \
+        'Maybe the client did not receive any messages?'


Not sure what the failure criteria is here but it looks like the assert message could use an update

mikaelarguedas · 2017-11-30T19:28:05Z

logging_demo/test/logging_demo_main_debug_severity.regex

@@ -0,0 +1 @@
+\[DEBUG\] \[logger_usage_demo\]: Count divides into 12 \(function evaluated to true\)


Doest this need to be a regex? it seems like it could be a string to match like the file below

if it's a txt it can't have any messages printed before this one gets matched or it fails

mikaelarguedas · 2017-11-30T23:35:46Z

logging_demo/package.xml

@@ -0,0 +1,36 @@
+<?xml version="1.0"?>
+<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+<package format="2">


Now that ros2/common_interfaces#45 and friends have been merged this should be updated to format3 and be declared as part of the rosidl_interface_packages group

This reverts commit 47305c0.

dhood · 2017-12-04T01:48:56Z

still looking for reviews on this if anyone has a chance, but it's lower priority overall since it's just a demo (the changes to the core required for this have been merged already)

wjwwood

lgtm, though I think it might need some touch ups to compile again after ros2/rcutils#82

wjwwood · 2017-12-04T02:48:55Z

logging_demo/README.md

@@ -0,0 +1,75 @@
+# Logging and logger configuration
+
+See [the logging page]() for details on available functionality.


Needs a link?

yeah I'll add it when I move it to the wiki 👍 sorry it wasn't clear that this is temporary.

wjwwood · 2017-12-04T02:52:51Z

logging_demo/logging_wiki.md

+
+In C++:
+```
+rcutils_logging_set_logger_severity_threshold("logger_name", RCUTILS_LOG_SEVERITY_DEBUG);


Are these names still right?

wjwwood · 2017-12-04T02:55:20Z

logging_demo/src/logger_config_component.cpp

+      }
+
+      // TODO(dhood): allow configuration through rclcpp
+      auto ret = rcutils_logging_set_logger_severity_threshold(


These changed in ros2/rcutils#82 didn't they?

wjwwood · 2017-12-04T02:56:45Z

logging_demo/src/logger_config_component.cpp

+: Node("logger_config")
+{
+  auto handle_logger_config_req =
+    [this](


You only ever appear to use the get_name() of this, maybe just capture the name?

Or make it a method like you did elsewhere in this pr.

dhood · 2017-12-04T04:45:34Z

windows CI (only this package):

dhood · 2017-12-04T06:03:08Z

@nuclearsandwich apologies for forgetting to tag you earlier but this (logging_demo) is a new package this release

dhood · 2017-12-04T15:51:56Z

@mikaelarguedas (build cop) I've seen the instability with connext and will fix

dhood · 2017-12-06T23:01:46Z

btw the wiki page is at https://github.com/ros2/ros2/wiki/Logging-and-logger-configuration

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

dhood self-assigned this Nov 22, 2017

dhood mentioned this pull request Nov 25, 2017

Only evaluate logging 'function'/'expression' if severity is enabled ros2/rcutils#75

Merged

dhood force-pushed the logging_config_demo branch from 790d9db to d6a0b66 Compare November 28, 2017 03:16

dhood added 17 commits November 28, 2017 12:25

First commit of logger config server component

71709a6

Use package name for visibility macros

9425ba0

Add logger usage component

8798484

Add executable that composes th e two nodes

8841e43

Make the function to evaluate an attribute

4215bef

Add test

a45f493

printed -> logged

20b250a

Add test that debug output gets enabled

f454f22

Enable debug automatically after 5s instead of relying on service call

ebdf605

Linter fixup

e451278

WIP readme

f20eeed

Fixup

16dcf75

Fix tests for windows

e4f4dd4

Fix windows warning

6bc815b

Add some description to message of log calls with features

426ce10

Readme WIP

b106373

Break into 2 wiki pages

916b226

dhood force-pushed the logging_config_demo branch from 13e7e91 to 916b226 Compare November 28, 2017 20:26

Remove unrelated change

dde08f5

dhood force-pushed the logging_config_demo branch from 93e978e to dde08f5 Compare November 28, 2017 20:29

dhood mentioned this pull request Nov 28, 2017

Logging meta-ticket ros2/ros2#425

Closed

14 tasks

dhood added 2 commits November 28, 2017 12:45

Readme edits

486446b

Fix regex matching

f939305

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

mikaelarguedas reviewed Nov 30, 2017

View reviewed changes

dhood added 12 commits November 30, 2017 17:03

swap to logger obj

47305c0

Add comment why pedantic isn't possible

450941e

Add another reason

816cbfc

various fixups

c9d468c

Revert "swap to logger obj"

6d8df9a

This reverts commit 47305c0.

Check return value from rcutils

7983c11

Declare membership of rosidl_interface_packages group

e1da9e9

Fix launch error message

0182e22

Highlight logging_demo runtime config is temporary approach

5bcdec5

readme fixup

9533f7b

divides_into_twelve -> is_divisor_of_twelve

c81ba64

Missed some copyright years

a0933e3

typo

b6bc4d8

wjwwood approved these changes Dec 4, 2017

View reviewed changes

dhood added 5 commits December 3, 2017 20:17

Rename severity_threshold -> level

3e9e5b1

Use absolute path for generate (cmake 3.10)

9194721

Don't rely on escalated namespaces

9f27f84

Make service handler a member function

4d75160

Remove wiki pages (will go in wiki)

72f6c9b

dhood merged commit 930afc1 into master Dec 4, 2017

dhood deleted the logging_config_demo branch December 4, 2017 04:46

dhood removed the in review Waiting for review (Kanban column) label Dec 4, 2017

dhood mentioned this pull request Dec 4, 2017

Increase allowed time for logging demo test #204

Merged

		@@ -0,0 +1,37 @@
		// Copyright 2016 Open Source Robotics Foundation, Inc.

		launch(name, [executable], output_file)


		def launch(name, cmd, output_file, additional_processes=None):

		@@ -0,0 +1 @@
		\[DEBUG\] \[logger_usage_demo\]: Count divides into 12 \(function evaluated to true\)

		@@ -0,0 +1,75 @@
		# Logging and logger configuration

		See [the logging page]() for details on available functionality.

Add demo for logging + logger config #194

Add demo for logging + logger config #194

Conversation

dhood commented Nov 22, 2017

dhood commented Nov 28, 2017

mikaelarguedas left a 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

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

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

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 Dec 4, 2017

wjwwood left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Dec 4, 2017

dhood commented Dec 4, 2017

dhood commented Dec 4, 2017

dhood commented Dec 6, 2017