Unify logging #60

linaScience · 2023-04-26T07:56:19Z

No description provided.

Sohn123

I think the format you developed is good :) Just some minor corrections and nitpicks. So let's fix it and

Sohn123 · 2023-07-14T07:56:46Z

src/Configuration.cpp

@@ -44,9 +44,9 @@ void Configuration::set_all_available_sockets_if_empty() {
            }
        }
        if (socknames.empty()) {
-            throw std::runtime_error("No sockets were configured and no sockets could be found.");
+            throw std::runtime_error("Could not find and configure sockets.");


I think this message is misleading now. It should state that no sockets were configured.

Sohn123 · 2023-07-14T07:58:14Z

src/Configuration.cpp

        }
-        WMLOG(DEBUG) << "No sockets were configured. Using all " << socknames.size() << " sockets available.";
+        WMLOG(DEBUG) << "Could not configure sockets. Using all " << socknames.size() << " sockets available.";


This is not an error. There were no sockets specified and our auto-detection worked.

Does "Found" << socknames.size() << "available sockets". sound better?

Sohn123 · 2023-07-14T07:58:51Z

src/EventLoop.cpp

-                 << event->station.vlan_id.value_or(0);
-    WMLOG(INFO) << "Station " << event->station << " connected to AP for VXLAN " << event->station.vni()
-                << " at interface " << event->station.sockname;
+    WMLOG(DEBUG) << "Calling handle_connect() for MAC:" << event->station


We are not calling the method it is already called. Maybe rather use entering?

Sohn123 · 2023-07-14T08:00:15Z

src/EventLoop.cpp

        caller.deauth_station(event->station);
    }
 }

 void EventLoop::handle_disconnect(ipc::DisconnectEvent* event) {
-    WMLOG(INFO) << "Station " << event->station << " disconnected from AP";
+    WMLOG(INFO) << "Disconnected station: " << event->station.mac.string();


This sounds like we disconnected the station but rather the station decided to disconnect.

Sohn123 · 2023-07-14T08:01:03Z

src/ipc/Socket.cpp

@@ -74,29 +74,29 @@ ipc::Socket::Socket(const std::chrono::duration<int>& timeout, const std::string
    local.sun_path[sizeof(local.sun_path) - 1] = '\0';
    if (bind(sock_fd, reinterpret_cast<struct sockaddr*>(&local), sizeof(local)) == -1) {
        close(sock_fd);
-        throw std::runtime_error("could not bind to socket " + dest_path + ": " + std::strerror(errno));
+        throw std::runtime_error("Could not bind to socket " + dest_path + ": " + std::strerror(errno));


Colon missing.

Sohn123 · 2023-07-14T08:03:04Z

src/ipc/Socket.cpp

@@ -118,7 +118,7 @@ void ipc::Socket::send_command(const std::vector<std::string>& args) {

    ssize_t err = send(sock_fd, command.c_str(), command.size(), 0);
    if (err < 0) {
-        throw std::runtime_error(std::string("could not send_command to socket: ") + std::strerror(errno));
+        throw std::runtime_error(std::string("Could not send_command() to socket: ") + std::strerror(errno));


I would write Could not send to socket:

Sohn123 · 2023-07-14T08:04:46Z

src/nl/Socket.cpp

@@ -122,7 +122,7 @@ void nl::Socket::delete_interface(const std::string &name) {
    if (err < 0) {
        throw std::runtime_error(std::string("Could not delete link: ") + nl_geterror(err));
    }
-    WMLOG(INFO) << "Deleted " << name;
+    WMLOG(INFO) << "Deleted interface." << name;


Colon instead of dot

Sohn123 · 2023-07-14T11:40:33Z

Please also rebase onto latest main so the pipeline is green.

rgwohlbold · 2023-07-14T12:02:19Z

src/BridgePerVxlanRenderer.cpp


    if (not station.vlan_id.has_value()) {
-        throw std::runtime_error("The station " + station.mac.string() + " has no vlan_id");
+        throw std::runtime_error("Station with MAC: " + station.mac.string() + " has no vlan_id.");


In my opinion, we should use key-value pairs to give additional information, and move them to the end.
For this log message, I propose "Station has no vlan_id mac=", similar for the others

rgwohlbold

Our goal here is to make our logs searchable.
For example, if I wanted to find all events relating to a MAC address 00:00:00:00:00:01, I would search for station=00:00:00:00:00:01.
For that reason, I propose the following changes:

We should use unified keys, for example station instead of station or mac
No whitespace between the = and the value
We should put the key-value pairs last and leave out prepositions like with before the pairs.

rgwohlbold · 2023-07-14T13:44:36Z

src/nl/Socket.cpp

@@ -122,7 +122,7 @@ void nl::Socket::delete_interface(const std::string &name) {
    if (err < 0) {
        throw std::runtime_error(std::string("Could not delete link: ") + nl_geterror(err));
    }
-    WMLOG(INFO) << "Deleted " << name;
+    WMLOG(INFO) << "Deleted interface:" << name;


use iface=

rgwohlbold · 2023-07-14T13:45:02Z

src/nl/Socket.cpp

    }
-    WMLOG(INFO) << "Connected " << interface_name << " to " << bridge_name;
+    WMLOG(INFO) << "Connected interface=" << interface_name << " to bridge= " << bridge_name;


We should put the KV-pairs last

Would "Connected iface=" << interface_name << " bridge=" << bridge_name; fit here?
Or do you suggest something else for this case?

Sohn123

For me it is fine now. Thanks for all the work you put in @linaScience What do you think @rgwohlbold ?

rgwohlbold

LGTM

linaScience linked an issue Apr 26, 2023 that may be closed by this pull request

Improve logging structure #23

Closed

5 tasks

Sohn123 marked this pull request as draft April 26, 2023 09:09

Sohn123 changed the title ~~[Draft] Unify logging~~ Unify logging Apr 26, 2023

linaScience marked this pull request as ready for review July 13, 2023 13:31

Sohn123 requested changes Jul 14, 2023

View reviewed changes

rgwohlbold reviewed Jul 14, 2023

View reviewed changes

linaScience and others added 5 commits July 14, 2023 14:55

Unify logging

c85c708

Format

33390d9

improve logging

aa957ee

fix linting errors

c11b2b5

format

59fb935

linaScience force-pushed the 23-improve-logging-structure branch from 518c2b9 to 59fb935 Compare July 14, 2023 13:24

apply comments

e6917f4

rgwohlbold requested changes Jul 14, 2023

View reviewed changes

linaScience added 6 commits July 17, 2023 13:44

put kv pairs last

60e48d5

format

24b41f5

insert string

9bd1624

improve language

7a3ff31

resolve binary operand error

41a3546

format

c51bebe

Sohn123 requested review from rgwohlbold and Sohn123 July 18, 2023 07:57

Sohn123 approved these changes Jul 18, 2023

View reviewed changes

rgwohlbold approved these changes Jul 18, 2023

View reviewed changes

linaScience merged commit 323a448 into main Jul 18, 2023

linaScience deleted the 23-improve-logging-structure branch July 18, 2023 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify logging #60

Unify logging #60

linaScience commented Apr 26, 2023

Sohn123 left a comment

Sohn123 Jul 14, 2023

Sohn123 Jul 14, 2023

linaScience Jul 14, 2023

Sohn123 Jul 14, 2023

Sohn123 Jul 14, 2023

Sohn123 Jul 14, 2023

Sohn123 Jul 14, 2023

Sohn123 Jul 14, 2023

Sohn123 commented Jul 14, 2023

rgwohlbold Jul 14, 2023

rgwohlbold left a comment

rgwohlbold Jul 14, 2023

rgwohlbold Jul 14, 2023

linaScience Jul 17, 2023

Sohn123 left a comment

rgwohlbold left a comment

Unify logging #60

Unify logging #60

Conversation

linaScience commented Apr 26, 2023

Sohn123 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

Sohn123 commented Jul 14, 2023

Choose a reason for hiding this comment

rgwohlbold 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

Sohn123 left a comment

Choose a reason for hiding this comment

rgwohlbold left a comment

Choose a reason for hiding this comment