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

[Logging] Introducing Provider/Receiver pattern for logging #1831

Merged
merged 24 commits into from
Dec 9, 2024

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Dec 2, 2024

Description

Split of the ecal logging mechanism into a provider and receiver class. Receiver class is an UDP receiver for all log messages. The receiving can be enabled/disabled by config. Additionally, it must be stated explicitly that the receiving component needs to be initialized when eCAL::Initialize(...) is called. Receiving is by default disabled.

Tests and apps adapted towards this logic.

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

I know it's still a draft, but I just read over it quickly and wanted to post a few things which came to my mind, before I forget them.

ecal/core/src/ecal_global_accessors.h Outdated Show resolved Hide resolved
ecal/core/src/ecal_globals.cpp Show resolved Hide resolved
@Peguen Peguen changed the title [Logging] Introducing Provider/Receiver pattern for lagging [Logging] Introducing Provider/Receiver pattern for logging Dec 2, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 68. Check the log or trigger a new build to see more.

ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
@Peguen Peguen added the cherry-pick-to-NONE Don't cherry-pick these changes label Dec 2, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 48. Check the log or trigger a new build to see more.

ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Show resolved Hide resolved
ecal/core/src/logging/ecal_log_provider.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 29. Check the log or trigger a new build to see more.

#include <string>

#include <ecal/ecal_log_level.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header ecal_log_level.h is not used directly [misc-include-cleaner]

Suggested change

const std::string tstring = get_time_str();

m_logfile_name = m_attributes.file.path + tstring + "_" + m_attributes.unit_name + "_" + std::to_string(m_attributes.process_id) + ".log";
m_logfile = fopen(m_logfile_name.c_str(), "w");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: assigning newly created 'gsl::owner<>' to non-owner 'FILE *' (aka '_IO_FILE *') [cppcoreguidelines-owning-memory]

      m_logfile = fopen(m_logfile_name.c_str(), "w");
      ^

const std::string tstring = get_time_str();

m_logfile_name = m_attributes.file.path + tstring + "_" + m_attributes.unit_name + "_" + std::to_string(m_attributes.process_id) + ".log";
m_logfile = fopen(m_logfile_name.c_str(), "w");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "fopen" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_provider.cpp:23:

- #include <ecal/ecal_time.h>
+ #include <cstdio>
+ #include <ecal/ecal_time.h>


bool CLogProvider::StartUDPLogging()
{
const eCAL::UDP::SSenderAttr attr = Logging::UDP::ConvertToIOUDPSenderAttributes(m_attributes.udp_sender);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::SSenderAttr" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_provider.cpp:20:

- #include "serialization/ecal_serialize_logging.h"
+ #include "io/udp/ecal_udp_sender_attr.h"
+ #include "serialization/ecal_serialize_logging.h"

bool CLogProvider::StartUDPLogging()
{
const eCAL::UDP::SSenderAttr attr = Logging::UDP::ConvertToIOUDPSenderAttributes(m_attributes.udp_sender);
m_udp_logging_sender = std::make_unique<eCAL::UDP::CSampleSender>(attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::CSampleSender" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_provider.cpp:20:

- #include "serialization/ecal_serialize_logging.h"
+ #include "io/udp/ecal_udp_sample_sender.h"
+ #include "serialization/ecal_serialize_logging.h"

{
namespace Logging
{
CLogReceiver::CLogReceiver(const SReceiverAttributes& attr_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::SReceiverAttributes" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:22:

- #include "serialization/ecal_serialize_logging.h"
+ #include "logging/config/attributes/ecal_log_receiver_attributes.h"
+ #include "serialization/ecal_serialize_logging.h"

if (m_attributes.udp_enabled && m_attributes.receive_enabled)
{
// set logging receive network attributes
const eCAL::UDP::SReceiverAttr attr = Logging::UDP::ConvertToIOUDPReceiverAttributes(m_attributes.udp_receiver);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::SReceiverAttr" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:22:

- #include "serialization/ecal_serialize_logging.h"
+ #include "io/udp/ecal_udp_receiver_attr.h"
+ #include "serialization/ecal_serialize_logging.h"

const eCAL::UDP::SReceiverAttr attr = Logging::UDP::ConvertToIOUDPReceiverAttributes(m_attributes.udp_receiver);

// start logging receiver
m_log_receiver = std::make_shared<eCAL::UDP::CSampleReceiver>(attr, std::bind(&CLogReceiver::HasSample, this, std::placeholders::_1), std::bind(&CLogReceiver::ApplySample, this, std::placeholders::_1, std::placeholders::_2));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::CSampleReceiver" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:22:

- #include "serialization/ecal_serialize_logging.h"
+ #include "io/udp/ecal_udp_sample_receiver.h"
+ #include "serialization/ecal_serialize_logging.h"

const eCAL::UDP::SReceiverAttr attr = Logging::UDP::ConvertToIOUDPReceiverAttributes(m_attributes.udp_receiver);

// start logging receiver
m_log_receiver = std::make_shared<eCAL::UDP::CSampleReceiver>(attr, std::bind(&CLogReceiver::HasSample, this, std::placeholders::_1), std::bind(&CLogReceiver::ApplySample, this, std::placeholders::_1, std::placeholders::_2));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::bind" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:24:

- #include <iostream>
+ #include <functional>
+ #include <iostream>

const eCAL::UDP::SReceiverAttr attr = Logging::UDP::ConvertToIOUDPReceiverAttributes(m_attributes.udp_receiver);

// start logging receiver
m_log_receiver = std::make_shared<eCAL::UDP::CSampleReceiver>(attr, std::bind(&CLogReceiver::HasSample, this, std::placeholders::_1), std::bind(&CLogReceiver::ApplySample, this, std::placeholders::_1, std::placeholders::_2));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::make_shared" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:25:

+ #include <memory>

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 36. Check the log or trigger a new build to see more.

@@ -71,7 +71,7 @@ Ecalmon::Ecalmon(QWidget *parent)
, monitor_error_counter_(0)
{
// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Default" is directly included [misc-include-cleaner]

app/mon/mon_gui/src/ecalmon.cpp:24:

- #include "widgets/about_dialog/about_dialog.h"
+ #include "ecal/ecal_init.h"
+ #include "widgets/about_dialog/about_dialog.h"

@@ -71,7 +71,7 @@
, monitor_error_counter_(0)
{
// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Monitoring" is directly included [misc-include-cleaner]

  eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                            ^

@@ -71,7 +71,7 @@
, monitor_error_counter_(0)
{
// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

  eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                                                     ^

@@ -71,7 +71,7 @@
, monitor_error_counter_(0)
{
// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALMon", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Initialize" is directly included [misc-include-cleaner]

app/mon/mon_gui/src/ecalmon.cpp:24:

- #include "widgets/about_dialog/about_dialog.h"
+ #include "ecal/ecal_core.h"
+ #include "widgets/about_dialog/about_dialog.h"

@@ -32,7 +32,7 @@ int main(int argc, char** argv)
{
auto args = ParseArgs(argc, argv);

auto status = eCAL::Initialize(0, nullptr, "eCALMon TUI", eCAL::Init::Default | eCAL::Init::Monitoring);
auto status = eCAL::Initialize(0, nullptr, "eCALMon TUI", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Default" is directly included [misc-include-cleaner]

app/mon/mon_tui/src/main.cpp:28:

- #include "tui/tui.hpp"
+ #include "ecal/ecal_init.h"
+ #include "tui/tui.hpp"

@@ -136,7 +136,7 @@
}

// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALRecGUI", eCAL::Init::Default | eCAL::Init::Service | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRecGUI", eCAL::Init::Default | eCAL::Init::Service | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

  eCAL::Initialize(0, nullptr, "eCALRecGUI", eCAL::Init::Default | eCAL::Init::Service | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                                                                              ^

@@ -136,7 +136,7 @@
}

// Just make sure that eCAL is initialized
eCAL::Initialize(0, nullptr, "eCALRecGUI", eCAL::Init::Default | eCAL::Init::Service | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRecGUI", eCAL::Init::Default | eCAL::Init::Service | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Initialize" is directly included [misc-include-cleaner]

app/rec/rec_gui/src/main.cpp:19:

- #include "ecalrec_gui.h"
+ #include "ecal/ecal_core.h"
+ #include "ecalrec_gui.h"

@@ -60,7 +60,7 @@ namespace eCAL
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Default" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_init.h"
+ #include "recorder_settings.h"

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Monitoring" is directly included [misc-include-cleaner]

      eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                         ^

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

      eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                                                  ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 22. Check the log or trigger a new build to see more.

@@ -60,7 +60,7 @@ namespace eCAL
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Initialize" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_core.h"
+ #include "recorder_settings.h"

}
}

if ((components_ & Init::UDPLogReceive) != 0u)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

    if ((components_ & Init::UDPLogReceive) != 0u)
                             ^

{
if (log_udp_receiver_instance == nullptr)
{
log_udp_receiver_instance = std::make_unique<Logging::CLogReceiver>(eCAL::Logging::BuildLoggingReceiverAttributes(GetLoggingConfiguration(), GetRegistrationConfiguration(), GetTransportLayerConfiguration()));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::CLogReceiver" is directly included [misc-include-cleaner]

ecal/core/src/ecal_globals.cpp:24:

+ #include "logging/ecal_log_receiver.h"

@@ -25,7 +25,7 @@ namespace eCAL
{
namespace UDP
{
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SUDPSender& sender_attr_)
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SProviderAttributes::SUDPSender& sender_attr_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::SProviderAttributes" is directly included [misc-include-cleaner]

ecal/core/src/logging/config/builder/udp_attribute_builder.cpp:20:

+ #include "logging/config/attributes/ecal_log_provider_attributes.h"

@@ -25,7 +25,7 @@
{
namespace UDP
{
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SUDPSender& sender_attr_)
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SProviderAttributes::SUDPSender& sender_attr_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::SSenderAttr" is directly included [misc-include-cleaner]

ecal/core/src/logging/config/builder/udp_attribute_builder.cpp:20:

+ #include "io/udp/ecal_udp_sender_attr.h"


// serialize message list
{
const std::lock_guard<std::mutex> lock(m_log_mtx);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::lock_guard" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:25:

+ #include <mutex>


// serialize message list
{
const std::lock_guard<std::mutex> lock(m_log_mtx);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::mutex" is directly included [misc-include-cleaner]

        const std::lock_guard<std::mutex> lock(m_log_mtx);
                                   ^

}
}

bool CLogReceiver::HasSample(const std::string& sample_name_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'HasSample' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/logging/ecal_log_receiver.h:78:

-         bool HasSample(const std::string& sample_name_);
+         static bool HasSample(const std::string& sample_name_);

return (sample_name_ == "_log_message_");
}

bool CLogReceiver::ApplySample(const char* serialized_sample_data_, size_t serialized_sample_size_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:24:

- #include <iostream>
+ #include <cstddef>
+ #include <iostream>

bool CLogReceiver::ApplySample(const char* serialized_sample_data_, size_t serialized_sample_size_)
{
// TODO: Limit maximum size of collected log messages !
Logging::SLogMessage log_message;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::SLogMessage" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.cpp:22:

- #include "serialization/ecal_serialize_logging.h"
+ #include "ecal/types/logging.h"
+ #include "serialization/ecal_serialize_logging.h"

@Peguen Peguen marked this pull request as ready for review December 4, 2024 16:02
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

std::string get_time_str()
{
char fmt[64];
struct timeval tv;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "timeval" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_provider.cpp:23:

- #include <ecal/ecal_time.h>
+ #include <bits/types/struct_timeval.h>
+ #include <ecal/ecal_time.h>

{
namespace Logging
{
class CLogReceiver
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'CLogReceiver' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

    class CLogReceiver
          ^


private:
bool HasSample(const std::string& sample_name_);
bool ApplySample(const char* serialized_sample_data_, size_t serialized_sample_size_);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_receiver.h:28:

- #include <ecal/types/logging.h>
+ #include <cstddef>
+ #include <ecal/types/logging.h>

@@ -129,7 +130,7 @@ TEST(logging_to /*unused*/, udp /*unused*/)
const std::string log_message = "Logging to udp test.";
auto ecal_config = GetUDPConfiguration();

eCAL::Initialize(ecal_config, unit_name.c_str(), eCAL::Init::Logging);
eCAL::Initialize(ecal_config, unit_name.c_str(), eCAL::Init::Logging | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

  eCAL::Initialize(ecal_config, unit_name.c_str(), eCAL::Init::Logging | eCAL::Init::UDPLogReceive);  
                                                                                     ^

@Peguen Peguen force-pushed the feature/logging_split branch from 7c3cfe5 to ee26111 Compare December 5, 2024 08:54
Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

I think the PR is in a good state. The only thing that I would change is the Config Object.
It feels wrong that the UDP sink has the property receive, I think I would rather split it.

The internal handling could possibly be changed later, also.

ecal/core/include/ecal/config/logging.h Outdated Show resolved Hide resolved
{
std::string address;
int port;
bool broadcast;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does a receiver need a broadcast argument? I thought it's only relevant for sending infos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because of the eCAL::UDP::SReceiverAttr in io/udp/ , it expects a value also the broadcast value.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -60,7 +60,7 @@ namespace eCAL
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Default" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_init.h"
+ #include "recorder_settings.h"

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Monitoring" is directly included [misc-include-cleaner]

      eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                                       ^

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::UDPLogReceive" is directly included [misc-include-cleaner]

      eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
                                                                                                                ^

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize(0, nullptr, "eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring | eCAL::Init::UDPLogReceive);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Initialize" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_core.h"
+ #include "recorder_settings.h"

@@ -576,6 +576,22 @@ namespace YAML
/____/\___/\_, /\_, /_/_//_/\_, /
/___//___/ /___/
*/

Node convert<eCAL::Logging::Sinks::UDPReceiver::Configuration>::encode(const eCAL::Logging::Sinks::UDPReceiver::Configuration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDPReceiver::Configuration" is directly included [misc-include-cleaner]

ecal/core/src/config/configuration_to_yaml.cpp:1:

+ #include "ecal/config/logging.h"

@@ -297,6 +297,14 @@ namespace YAML
/____/\___/\_, /\_, /_/_//_/\_, /
/___//___/ /___/
*/
template<>
struct convert<eCAL::Logging::Sinks::UDPReceiver::Configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDPReceiver::Configuration" is directly included [misc-include-cleaner]

ecal/core/src/config/configuration_to_yaml.h:26:

- #include <ecal/config/configuration.h>
+ #include "ecal/config/logging.h"
+ #include <ecal/config/configuration.h>

@@ -25,7 +25,7 @@ namespace eCAL
{
namespace UDP
{
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SUDPSender& sender_attr_)
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SProviderAttributes::SUDP& sender_attr_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::SProviderAttributes" is directly included [misc-include-cleaner]

ecal/core/src/logging/config/builder/udp_attribute_builder.cpp:20:

+ #include "logging/config/attributes/ecal_log_provider_attributes.h"

@@ -25,7 +25,7 @@
{
namespace UDP
{
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SUDPSender& sender_attr_)
eCAL::UDP::SSenderAttr ConvertToIOUDPSenderAttributes(const Logging::SProviderAttributes::SUDP& sender_attr_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::SSenderAttr" is directly included [misc-include-cleaner]

ecal/core/src/logging/config/builder/udp_attribute_builder.cpp:20:

+ #include "io/udp/ecal_udp_sender_attr.h"


const std::string tstring = get_time_str();

m_logfile_name = m_attributes.file_config.path + tstring + "_" + m_attributes.unit_name + "_" + std::to_string(m_attributes.process_id) + ".log";
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::to_string" is directly included [misc-include-cleaner]

      m_logfile_name = m_attributes.file_config.path + tstring + "_" + m_attributes.unit_name + "_" + std::to_string(m_attributes.process_id) + ".log";
                                                                                                           ^


bool CLogProvider::StartUDPLogging()
{
const eCAL::UDP::SSenderAttr attr = Logging::UDP::ConvertToIOUDPSenderAttributes(m_attributes.udp_config);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::UDP::SSenderAttr" is directly included [misc-include-cleaner]

ecal/core/src/logging/ecal_log_provider.cpp:20:

- #include "serialization/ecal_serialize_logging.h"
+ #include "io/udp/ecal_udp_sender_attr.h"
+ #include "serialization/ecal_serialize_logging.h"

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -60,7 +60,7 @@ namespace eCAL
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize("", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize("eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Default" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_init.h"
+ #include "recorder_settings.h"

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize("", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize("eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Init::Monitoring" is directly included [misc-include-cleaner]

      eCAL::Initialize("eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
                                                                           ^

@@ -60,7 +60,7 @@
settings_.ClearHostFilter(); // There is no global host filter

// Initialize eCAL
eCAL::Initialize("", eCAL::Init::Default | eCAL::Init::Monitoring);
eCAL::Initialize("eCALRec-Server", eCAL::Init::Default | eCAL::Init::Monitoring);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Initialize" is directly included [misc-include-cleaner]

app/rec/rec_server_core/src/rec_server_impl.cpp:21:

- #include "recorder_settings.h"
+ #include "ecal/ecal_core.h"
+ #include "recorder_settings.h"

std::string path { "" }; //!< Path to log file (Default: "")
eCAL_Logging_Filter filter_log_file { log_level_none }; /*!< Log messages logged into file system (all, info, warning, error, fatal, debug1, debug2, debug3, debug4)
(Default: info, warning, error, fatal)*/
std::string path { "" }; //!< Path to log file (Default: "")
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant string initialization [readability-redundant-string-init]

Suggested change
std::string path { "" }; //!< Path to log file (Default: "")
std::string path; //!< Path to log file (Default: "")

Node convert<eCAL::Logging::Sinks::UDP::Configuration>::encode(const eCAL::Logging::Sinks::UDP::Configuration& config_)

Node convert<eCAL::Logging::Sinks::UDP::ReceiverConfiguration>::encode(const eCAL::Logging::Sinks::UDP::ReceiverConfiguration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDP::ReceiverConfiguration" is directly included [misc-include-cleaner]

ecal/core/src/config/configuration_to_yaml.cpp:1:

+ #include "ecal/config/logging.h"

@@ -298,19 +298,19 @@ namespace YAML
/___//___/ /___/
*/
template<>
struct convert<eCAL::Logging::Sinks::UDP::Configuration>
struct convert<eCAL::Logging::Sinks::UDP::ReceiverConfiguration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDP::ReceiverConfiguration" is directly included [misc-include-cleaner]

ecal/core/src/config/configuration_to_yaml.h:26:

- #include <ecal/config/configuration.h>
+ #include "ecal/config/logging.h"
+ #include <ecal/config/configuration.h>

};

template<>
struct convert<eCAL::Logging::Sinks::Console::Configuration>
struct convert<eCAL::Logging::Sinks::UDP::ProviderConfiguration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDP::ProviderConfiguration" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Sinks::UDP::ProviderConfiguration>
                                            ^

@@ -321,6 +321,22 @@
static bool decode(const Node& node_, eCAL::Logging::Sinks::File::Configuration& config_);
};

template<>
struct convert<eCAL::Logging::Sinks::UDP::Configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::UDP::Configuration" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Sinks::UDP::Configuration>
                                            ^

};

template<>
struct convert<eCAL::Logging::Sinks::Sink>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Sinks::Sink" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Sinks::Sink>
                                       ^

eCAL_Logging_Filter GetConsoleLogFilter () { return GetConfiguration().logging.sinks.console.filter_log_con; }
eCAL_Logging_Filter GetFileLogFilter () { return GetConfiguration().logging.sinks.file.filter_log_file; }
eCAL_Logging_Filter GetUdpLogFilter () { return GetConfiguration().logging.sinks.udp.filter_log_udp; }
eCAL_Logging_Filter GetConsoleLogFilter () { return GetConfiguration().logging.sinks.console.filter_log; }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL_Logging_Filter" is directly included [misc-include-cleaner]

ecal/core/src/config/ecal_config.cpp:19:

- #include <ecal/ecal_config.h>
+ #include "ecal/ecal_log_level.h"
+ #include <ecal/ecal_config.h>

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

I just have a very few minor remarks, but I guess then finally ready to be merged 😄


File::Configuration file_config;
UDP::Configuration udp_config;

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks very clean like this!

static const unsigned int Service = 0x004;
static const unsigned int Monitoring = 0x008;
static const unsigned int Logging = 0x010;
static const unsigned int TimeSync = 0x020;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could realign, so you don't get any changes in this file


static bool decode(const Node& node_, eCAL::Logging::Sinks::UDP::Configuration& config_);
static bool decode(const Node& node_, eCAL::Logging::Sinks::UDP::ReceiverConfiguration& config_);
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit strange that ReceiverConfiguration is in the Sinks namespace.
Does eCAL::Logging::Receiver::UDP::Configuration make more sense?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Node convert<eCAL::Logging::Sinks::UDP::Configuration>::encode(const eCAL::Logging::Sinks::UDP::Configuration& config_)

Node convert<eCAL::Logging::Receiver::UDP::Configuration>::encode(const eCAL::Logging::Receiver::UDP::Configuration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Receiver::UDP::Configuration" is directly included [misc-include-cleaner]

ecal/core/src/config/configuration_to_yaml.cpp:1:

+ #include "ecal/config/logging.h"

std::vector<std::string> tmp;
AssignValue<std::vector<std::string>>(tmp, node_, "level");
config_.filter_log_udp = ParseLogLevel(tmp);
Node convert<eCAL::Logging::Receiver::Configuration>::encode(const eCAL::Logging::Receiver::Configuration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Receiver::Configuration" is directly included [misc-include-cleaner]

  Node convert<eCAL::Logging::Receiver::Configuration>::encode(const eCAL::Logging::Receiver::Configuration& config_)
                                        ^

return true;
}

Node convert<eCAL::Logging::Sinks::Console::Configuration>::encode(const eCAL::Logging::Sinks::Console::Configuration& config_)
Node convert<eCAL::Logging::Provider::UDP::Configuration>::encode(const eCAL::Logging::Provider::UDP::Configuration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::UDP::Configuration" is directly included [misc-include-cleaner]

  Node convert<eCAL::Logging::Provider::UDP::Configuration>::encode(const eCAL::Logging::Provider::UDP::Configuration& config_)
                                             ^

return true;
}

Node convert<eCAL::Logging::Provider::Sink>::encode(const eCAL::Logging::Provider::Sink& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::Sink" is directly included [misc-include-cleaner]

  Node convert<eCAL::Logging::Provider::Sink>::encode(const eCAL::Logging::Provider::Sink& config_)
                                        ^

return true;
}

Node convert<eCAL::Logging::Sinks::File::Configuration>::encode(const eCAL::Logging::Sinks::File::Configuration& config_)
Node convert<eCAL::Logging::Provider::File::Configuration>::encode(const eCAL::Logging::Provider::File::Configuration& config_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::File::Configuration" is directly included [misc-include-cleaner]

  Node convert<eCAL::Logging::Provider::File::Configuration>::encode(const eCAL::Logging::Provider::File::Configuration& config_)
                                              ^

};

template<>
struct convert<eCAL::Logging::Sinks::File::Configuration>
struct convert<eCAL::Logging::Provider::File::Configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::File::Configuration" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Provider::File::Configuration>
                                                ^

};

template<>
struct convert<eCAL::Logging::Sinks::Configuration>
struct convert<eCAL::Logging::Provider::UDP::Configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::UDP::Configuration" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Provider::UDP::Configuration>
                                               ^

};

template<>
struct convert<eCAL::Logging::Provider::Sink>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::Sink" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Provider::Sink>
                                          ^

};

template<>
struct convert<eCAL::Logging::Provider::Configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL::Logging::Provider::Configuration" is directly included [misc-include-cleaner]

  struct convert<eCAL::Logging::Provider::Configuration>
                                          ^

eCAL_Logging_Filter GetConsoleLogFilter () { return GetConfiguration().logging.sinks.console.filter_log_con; }
eCAL_Logging_Filter GetFileLogFilter () { return GetConfiguration().logging.sinks.file.filter_log_file; }
eCAL_Logging_Filter GetUdpLogFilter () { return GetConfiguration().logging.sinks.udp.filter_log_udp; }
eCAL_Logging_Filter GetConsoleLogFilter () { return GetConfiguration().logging.provider.console.filter_log; }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "eCAL_Logging_Filter" is directly included [misc-include-cleaner]

ecal/core/src/config/ecal_config.cpp:19:

- #include <ecal/ecal_config.h>
+ #include "ecal/ecal_log_level.h"
+ #include <ecal/ecal_config.h>

@Peguen Peguen merged commit 7ba8e0a into master Dec 9, 2024
20 checks passed
@Peguen Peguen deleted the feature/logging_split branch December 9, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants