Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add demo for logging + logger config #194

Merged
merged 38 commits into from
Dec 4, 2017
Merged

Add demo for logging + logger config #194

merged 38 commits into from
Dec 4, 2017

Conversation

dhood
Copy link
Member

@dhood dhood commented Nov 22, 2017

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 dhood added the in progress Actively being worked on (Kanban column) label Nov 22, 2017
@dhood dhood self-assigned this Nov 22, 2017
@dhood
Copy link
Member Author

dhood commented Nov 28, 2017

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

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

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

endif()

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these two lines look long

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 2017

#define LOGGING_DEMO__LOGGER_CONFIG_COMPONENT_HPP_

#include "logging_demo/visibility_control.h"
#include "logging_demo/srv/config_logger.hpp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: include order

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description should be updated, maintainer tag as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! will make @nuclearsandwich's blooming easier

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! ok then it stays!


configure_file(
test/test_logging_demo.py.in
test_logging_demo${target_suffix}.py.genexp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what does genexp stands for here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

bool divides_into_twelve(size_t val, std::string logger_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it means is_divisor_of_twelve, I'll update the name

launch(name, [executable], output_file)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid mutable defaults in general

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point, forgot about that 👍


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@dhood
Copy link
Member Author

dhood commented Dec 4, 2017

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)

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a link?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these names still right?

}

// TODO(dhood): allow configuration through rclcpp
auto ret = rcutils_logging_set_logger_severity_threshold(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: Node("logger_config")
{
auto handle_logger_config_req =
[this](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dhood
Copy link
Member Author

dhood commented Dec 4, 2017

windows CI (only this package): Build Status

@dhood dhood merged commit 930afc1 into master Dec 4, 2017
@dhood dhood deleted the logging_config_demo branch December 4, 2017 04:46
@dhood dhood removed the in review Waiting for review (Kanban column) label Dec 4, 2017
@dhood
Copy link
Member Author

dhood commented Dec 4, 2017

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

@dhood
Copy link
Member Author

dhood commented Dec 4, 2017

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

@dhood
Copy link
Member Author

dhood commented Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants