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

[20274] Validate the YAML configuration file on parsing #113

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 57 additions & 38 deletions ddsrecorder/src/cpp/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
#include <cpp_utils/types/Fuzzy.hpp>
#include <cpp_utils/utils.hpp>

#include <ddspipe_core/configuration/DdsPipeLogConfiguration.hpp>

#include <ddsrecorder_participants/recorder/logging/DdsRecorderLogConsumer.hpp>
#include <ddsrecorder_participants/recorder/output/FileTracker.hpp>
#include <ddsrecorder_yaml/recorder/CommandlineArgsRecorder.hpp>
Expand Down Expand Up @@ -180,6 +182,41 @@ CommandCode state_to_command(
}
}

void register_log_consumers(const eprosima::ddspipe::core::DdsPipeLogConfiguration& configuration)
{
eprosima::utils::Log::ClearConsumers();
eprosima::utils::Log::SetVerbosity(configuration.verbosity);

// Std Log Consumer
if (configuration.stdout_enable)
{
eprosima::utils::Log::RegisterConsumer(
std::make_unique<eprosima::utils::StdLogConsumer>(&configuration));
}

// DDS Recorder Log Consumer
if (configuration.publish.enable)
{
eprosima::utils::Log::RegisterConsumer(
std::make_unique<eprosima::ddsrecorder::participants::DdsRecorderLogConsumer>(&configuration));
}

// NOTE:
// It will not filter any log, so Fast DDS logs will be visible unless Fast DDS is compiled
// in non debug or with LOG_NO_INFO=ON.
// This is the easiest way to allow to see Warnings and Errors from Fast DDS.
// Change it when Log Module is independent and with more extensive API.
// utils::Log::SetCategoryFilter(std::regex("(DDSRECORDER)"));
Comment on lines +204 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this note make sense anymore?

}

int exit(ProcessReturnCode code)
{
// Delete the consumers before closing
eprosima::utils::Log::ClearConsumers();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this flush as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it is called in ClearConsumers , so I guess not.


return static_cast<int>(code);
}

int main(
int argc,
char** argv)
Expand All @@ -193,15 +230,15 @@ int main(

if (arg_parse_result == ProcessReturnCode::help_argument)
{
return static_cast<int>(ProcessReturnCode::success);
return exit(ProcessReturnCode::success);
}
else if (arg_parse_result == ProcessReturnCode::version_argument)
{
return static_cast<int>(ProcessReturnCode::success);
return exit(ProcessReturnCode::success);
}
else if (arg_parse_result != ProcessReturnCode::success)
{
return static_cast<int>(arg_parse_result);
return exit(arg_parse_result);
}

// Check file is in args, else get the default file
Expand All @@ -225,7 +262,7 @@ int main(
EPROSIMA_LOG_ERROR(
DDSRECORDER_ARGS,
"File '" << commandline_args.file_path << "' does not exist or it is not accessible.");
return static_cast<int>(ProcessReturnCode::required_argument_failed);
return exit(ProcessReturnCode::required_argument_failed);
}
}

Expand Down Expand Up @@ -256,34 +293,19 @@ int main(
commandline_args.timeout));
}

// Register the LogConsumers to log the YAML configuration errors
eprosima::ddspipe::core::DdsPipeLogConfiguration log_configuration;
log_configuration.set(eprosima::utils::VerbosityKind::Warning);

register_log_consumers(log_configuration);

/////
// DDS Recorder Initialization

// Load configuration from YAML
eprosima::ddsrecorder::yaml::RecorderConfiguration configuration(commandline_args.file_path, &commandline_args);

/////
// Logging
{
const auto log_configuration = configuration.ddspipe_configuration.log_configuration;

eprosima::utils::Log::ClearConsumers();
eprosima::utils::Log::SetVerbosity(log_configuration.verbosity);

// Std Log Consumer
if (log_configuration.stdout_enable)
{
eprosima::utils::Log::RegisterConsumer(
std::make_unique<eprosima::utils::StdLogConsumer>(&log_configuration));
}

// DDS Recorder Log Consumer
if (log_configuration.publish.enable)
{
eprosima::utils::Log::RegisterConsumer(
std::make_unique<eprosima::ddsrecorder::participants::DdsRecorderLogConsumer>(&log_configuration));
}
}
register_log_consumers(configuration.ddspipe_configuration.log_configuration);

// Verify that the configuration is correct
eprosima::utils::Formatter error_msg;
Expand Down Expand Up @@ -400,9 +422,12 @@ int main(
" command.");
}

// Reload YAML configuration file, in case it changed during STOPPED state
// NOTE: Changes to all (but controller specific) recorder configuration options are taken into account
configuration = eprosima::ddsrecorder::yaml::RecorderConfiguration(commandline_args.file_path);
if (prev_command != CommandCode::close)
{
// Reload YAML configuration file, in case it changed during STOPPED state
// NOTE: Changes to all (but controller specific) recorder configuration options are taken into account
configuration = eprosima::ddsrecorder::yaml::RecorderConfiguration(commandline_args.file_path);
}

// Create DDS Recorder
auto recorder = std::make_unique<DdsRecorder>(
Expand Down Expand Up @@ -571,23 +596,17 @@ int main(
"Error Loading DDS Recorder Configuration from file " << commandline_args.file_path <<
". Error message:\n " <<
e.what());
return static_cast<int>(ProcessReturnCode::execution_failed);
return exit(ProcessReturnCode::execution_failed);
}
catch (const eprosima::utils::InitializationException& e)
{
EPROSIMA_LOG_ERROR(DDSRECORDER_ERROR,
"Error Initializing DDS Recorder. Error message:\n " <<
e.what());
return static_cast<int>(ProcessReturnCode::execution_failed);
return exit(ProcessReturnCode::execution_failed);
}

logUser(DDSRECORDER_EXECUTION, "Finishing DDS Recorder execution correctly.");

// Force print every log before closing
eprosima::utils::Log::Flush();

// Delete the consumers before closing
eprosima::utils::Log::ClearConsumers();

return static_cast<int>(ProcessReturnCode::success);
return exit(ProcessReturnCode::success);
}
12 changes: 6 additions & 6 deletions ddsrecorder/src/cpp/user_interface/arguments_configuration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,9 @@ ProcessReturnCode parse_arguments(
break;

case optionIndex::ACTIVATE_DEBUG:
commandline_args.log_filter[utils::VerbosityKind::Error].set_value("");
commandline_args.log_filter[utils::VerbosityKind::Warning].set_value("DDSRECORDER");
commandline_args.log_filter[utils::VerbosityKind::Info].set_value("DDSRECORDER");
commandline_args.log_filter.error.set_value("");
commandline_args.log_filter.warning.set_value("DDSRECORDER");
commandline_args.log_filter.info.set_value("DDSRECORDER");
commandline_args.log_verbosity = utils::VerbosityKind::Info;
break;

Expand All @@ -261,9 +261,9 @@ ProcessReturnCode parse_arguments(
break;

case optionIndex::LOG_FILTER:
commandline_args.log_filter[utils::VerbosityKind::Error].set_value(opt.arg);
commandline_args.log_filter[utils::VerbosityKind::Warning].set_value(opt.arg);
commandline_args.log_filter[utils::VerbosityKind::Info].set_value(opt.arg);
commandline_args.log_filter.error.set_value(opt.arg);
commandline_args.log_filter.warning.set_value(opt.arg);
commandline_args.log_filter.info.set_value(opt.arg);
break;

case optionIndex::LOG_VERBOSITY:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class LogMonitorDdsRecorderStatusTest : public testing::Test

utils::BaseLogConfiguration log_conf;
log_conf.verbosity = utils::VerbosityKind::Info;
log_conf.filter[utils::VerbosityKind::Info].set_value("MONITOR_DATA");
log_conf.filter.info.set_value("MONITOR_DATA");

utils::Log::SetVerbosity(log_conf.verbosity);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include <ddspipe_core/configuration/DdsPipeConfiguration.hpp>
#include <ddspipe_core/configuration/MonitorConfiguration.hpp>
#include <ddspipe_core/types/dds/DomainId.hpp>
#include <ddspipe_core/types/dds/TopicQoS.hpp>
#include <ddspipe_core/types/topic/dds/DistributedTopic.hpp>
#include <ddspipe_core/types/topic/filter/IFilterTopic.hpp>
Expand Down Expand Up @@ -114,6 +115,10 @@ class DDSRECORDER_YAML_DllAPI RecorderConfiguration
const Yaml& yml,
const ddspipe::yaml::YamlReaderVersion& version);

void load_output_configuration_(
const Yaml& yml,
const ddspipe::yaml::YamlReaderVersion& version);

void load_controller_configuration_(
const Yaml& yml,
const ddspipe::yaml::YamlReaderVersion& version);
Expand Down
7 changes: 3 additions & 4 deletions ddsrecorder_yaml/src/cpp/recorder/CommandlineArgsRecorder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ namespace yaml {

CommandlineArgsRecorder::CommandlineArgsRecorder()
{
log_filter[utils::VerbosityKind::Info].set_value("DDSRECORDER", utils::FuzzyLevelValues::fuzzy_level_default);
log_filter[utils::VerbosityKind::Warning].set_value("DDSRECORDER",
utils::FuzzyLevelValues::fuzzy_level_default);
log_filter[utils::VerbosityKind::Error].set_value("", utils::FuzzyLevelValues::fuzzy_level_default);
log_filter.info.set_value("DDSRECORDER", utils::FuzzyLevelValues::fuzzy_level_default);
log_filter.warning.set_value("DDSRECORDER", utils::FuzzyLevelValues::fuzzy_level_default);
log_filter.error.set_value("", utils::FuzzyLevelValues::fuzzy_level_default);
}

bool CommandlineArgsRecorder::is_valid(
Expand Down
11 changes: 11 additions & 0 deletions ddsrecorder_yaml/src/cpp/recorder/YamlReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <set>

#include <mcap/mcap.hpp>

#include <ddspipe_yaml/YamlReader.hpp>
#include <ddspipe_yaml/YamlValidator.hpp>

#include <ddsrecorder_yaml/recorder/yaml_configuration_tags.hpp>

Expand All @@ -30,6 +33,14 @@ YamlReader::get<mcap::McapWriterOptions>(
const Yaml& yml,
const YamlReaderVersion version)
{
static const std::set<TagType> tags{
RECORDER_COMPRESSION_SETTINGS_ALGORITHM_TAG,
RECORDER_COMPRESSION_SETTINGS_LEVEL_TAG,
RECORDER_COMPRESSION_SETTINGS_FORCE_TAG,
};

YamlValidator::validate_tags(yml, tags);

mcap::McapWriterOptions mcap_writer_options{"ros2"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Inner tags are not validated right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe they are


// Parse optional compression algorithm
Expand Down
Loading
Loading