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

reduce log noise #140

Merged
merged 5 commits into from
Feb 9, 2024
Merged

reduce log noise #140

merged 5 commits into from
Feb 9, 2024

Conversation

garlick
Copy link
Member

@garlick garlick commented Feb 9, 2024

This tightens up powerman logging and eliminates some unused stuff:

  • eliminates unused daemon mode (and -f,--foreground option to disable it)
  • eliminates direct syslog logging
  • reduces log noise for common events that won't scale to a large number of devices
  • apply some file descriptor hygiene that was poorly managed in a recent PR

@chu11
Copy link
Member

chu11 commented Feb 9, 2024

eliminates unused daemon mode (and -f,--foreground option to disable it)

A) There's the part of me that thinks it should stay, almost exclusively for the fact that the tool is still suffixed with a d. Someone out there will still expect it to happen / be available? Or atleast have an option for it? Ehh, take this comment for what it's worth. I know I tend to be more conservative on these kinds of decisions.

B) This seems like a big enough change, I think it should be another PR.

Comment on lines -262 to -266
TcpDev *tcp;

assert(dev->connect_state == DEV_CONNECTING
|| dev->connect_state == DEV_CONNECTED);
tcp = (TcpDev *)dev->data;
Copy link
Member

Choose a reason for hiding this comment

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

this code removal doesn't seem related to the "reduce log noise" commit. Seems like it's just dead code, meant for another commit and forgot it was in this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I changed some logging code and as a result, tcp became a set but not used error. The full diff is

@ -259,11 +255,8 @@ bool tcp_connect(Device * dev)
  */
 void tcp_disconnect(Device * dev)
 {
-    TcpDev *tcp;
-
     assert(dev->connect_state == DEV_CONNECTING
            || dev->connect_state == DEV_CONNECTED);
-    tcp = (TcpDev *)dev->data;
 
     dbg(DBG_DEVICE, "tcp_disconnect: %s on fd %d", dev->name, dev->fd);
 
@@ -274,8 +267,7 @@ void tcp_disconnect(Device * dev)
         dev->fd = NO_FD;
     }
 
-    if (!tcp->quiet)
-        err(false, "tcp_disconnect(%s): disconnected", dev->name);
+    dbg(DBG_DEVICE, "tcp_disconnect(%s): disconnected", dev->name);
 }
 
 void tcp_preprocess(Device *dev)

@garlick
Copy link
Member Author

garlick commented Feb 9, 2024

A) There's the part of me that thinks it should stay, almost exclusively for the fact that the tool is still suffixed with a d. Someone out there will still expect it to happen / be available? Or atleast have an option for it? Ehh, take this comment for what it's worth. I know I tend to be more conservative on these kinds of decisions.

It's a fair bit of code to keep around with no motivating use case...

B) This seems like a big enough change, I think it should be another PR.

Sure, that seems fine.

@garlick
Copy link
Member Author

garlick commented Feb 9, 2024

OK this is now based on top of #141

Problem: in the recent conversion of coprocesses from forkpty(3) to
socketpair(2), the unused side of the pair is not closed in the
parent, which causes file descriptors to leak as coprocesses are
restarted on error.

Close the file descriptor in the parent.

Close its counterpart in the child (just good hygiene, not a leak).
Problem: when we run the test suite, on/off messages appear
in syslog.

There is no longer a need to log anything directly to syslog.
When run under systemd, the systemd journal captures stderr
(and routes to syslog if so configured).

Log those messages to stderr.
Use a <level> prefix to preserve the ability to set the severity
of those messages in the config file.

Fixes chaos#138
Problem: when there are many devices, logging connect/disconnect
can produce a lot of log noise.

Turn these messages into DBG_DEVICE messages.

Fixes chaos#139
Problem: client connect/disconnect logging is not that useful.

Don't log when EOF or XPOLLHUP is received from a client.
Problem: dbg_notty() and error_notty() are no longer used
now that the powerman server no longer forks.

Drop those functions and associated syslog logging methods.
@garlick
Copy link
Member Author

garlick commented Feb 9, 2024

Tidied up and rebased on master.

Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

Lgtm!

@mergify mergify bot merged commit 43fc57c into chaos:master Feb 9, 2024
8 checks passed
@garlick garlick deleted the issue#139 branch February 9, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants